-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Installer.Remove
doesn't cleanup all files
#197
Comments
Good catch, thanks for the report! I remember thinking about this issue briefly when reviewing #148 but then I set that aside. I'm not sure if we have any confidence at all about other future files being added to release archives that would break things again. That aside, I wonder if we should also introduce an option similar to the one we have in Broadly speaking we've always been making the assumption that there are no conflicting files/names in the directory where the product is being installed but I'm not sure how safe that assumption is with more files being added. 🤔 |
Yes, this is a little bit subtle so I'm going to write down my assumptions before proposing the actual code change.
|
Yes, but I would double check with RelEng or someone internally to make sure this is a safe assumption to make.
Yes, by default. I think longer term we'd probably want to introduce a directory path as an option the consumers can specify. I expect it wouldn't change the default behaviour though.
Yes, but nothing more than that. Which is to say that I would avoid using |
For additional context, terraform-provider-* archives may include |
We see a test failure on terraform-ls because a non-empty directory cannot be removed:
https://github.com/hashicorp/terraform-ls/actions/runs/8819490049/job/24210822090?pr=1691#step:7:51
My first guess is that this is because the Terraform 1.8.2 release now includes a
LICENSE.txt
file that is not removed.Proposal
Include the license file in the removable sources.
A workaround would be to use
os.RemoveAll
instead ofos.Remove
in our tests. But I would still like to propose that hc-install should clean up all the files it creates.The text was updated successfully, but these errors were encountered: