Installer.Remove doesn't cleanup all files

[dbanck]

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 of os.Remove in our tests. But I would still like to propose that hc-install should clean up all the files it creates.

[radeksimko]

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 EnterpriseOptions, i.e. LicenseDir while we are at it.

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. 🤔

[kmoe]

Yes, this is a little bit subtle so I'm going to write down my assumptions before proposing the actual code change.

1. Any zip on releases.hashicorp.com for any product might contain a LICENSE.txt file. (In reality this is only true for certain releases, e.g. Terraform v1.8.2+)
2. By default, LICENSE.txt should be extracted to the same directory as the installed binary.
3. Installer.Remove should remove any file that the Installer itself created.

[radeksimko]

1. Any zip on releases.hashicorp.com for any product might contain a LICENSE.txt file. (In reality this is only true for certain releases, e.g. Terraform v1.8.2+)

Yes, but I would double check with RelEng or someone internally to make sure this is a safe assumption to make.

2. By default, LICENSE.txt should be extracted to the same directory as the installed binary.

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.

3. Installer.Remove should remove any file that the Installer itself created.

Yes, but nothing more than that. Which is to say that I would avoid using os.RemoveAll in this context. Instead we should collect the specific filenames and run os.Remove on each individually.

[bflad]

Any zip on releases.hashicorp.com for any product might contain a LICENSE.txt file. (In reality this is only true for certain releases, e.g. Terraform v1.8.2+)

For additional context, terraform-provider-* archives may include LICENSE (without the .txt), no one has specifically told us it needed the .txt file extension. I know terraform-provider-* installation is not a goal of this project, but I would be very cautious to assume about specific file names now or in the future.