Skip to content

Conversation

@amoeba
Copy link
Member

@amoeba amoeba commented Sep 9, 2025

This primarily fixes a bug with dbc uninstall for manifest-only drivers where the extra folder we install alongside the driver manifest wasn't being removed. I also factored out the common logic related to removing the driver shared library and added some defensive checks to limit the impact of a maliciously-crafted driver manifest.

Ref #15

@zeroshade zeroshade force-pushed the feat/manifest-only-drivers branch from 8d0f921 to 8b82752 Compare September 9, 2025 20:25
@amoeba amoeba requested a review from zeroshade September 15, 2025 19:56
@amoeba
Copy link
Member Author

amoeba commented Sep 15, 2025

Ready for a review. This now uses Clean on the path and I was able to test the new logic in config.UninstallDriverShared.

@zeroshade
Copy link
Member

Looks like there's still issues with the windows tests

@amoeba
Copy link
Member Author

amoeba commented Sep 16, 2025

I'm seeing if I can figure out what's going on in CI by surfacing the error (which I should have done originally).

@amoeba
Copy link
Member Author

amoeba commented Sep 16, 2025

Hrm,

Failed to rename sidecar folder. Something is wrong with this test: rename C:\Users\RUNNER1\AppData\Local\Temp\TestSubcommandsTestUninstallManifestOnlyDriver3321879900\001\test-driver-manifest-only C:\Users\RUNNER1\AppData\Local\Temp\TestSubcommandsTestUninstallManifestOnlyDriver3321879900\001\test-driver-manifest-only_windows_amd64_v1.0.0: The system cannot find the file specified.

I may need to log into my Windows machine to see if installing a manifest-only driver works on Windows.

@amoeba
Copy link
Member Author

amoeba commented Sep 16, 2025

Looks good now @zeroshade.

@zeroshade zeroshade dismissed their stale review September 16, 2025 20:45

incorrect suggestions

@amoeba amoeba merged commit 923adf0 into columnar-tech:main Sep 16, 2025
7 checks passed
@amoeba
Copy link
Member Author

amoeba commented Sep 16, 2025

Thanks for the review @zeroshade!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants