Skip to content
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

Add downloaded zip file to paths to be removed #74

Merged
merged 2 commits into from
Nov 17, 2022

Conversation

alenkacz
Copy link
Contributor

Currently, this file is written to tmp root with a random name and is not cleaned.

Another option would be to call os.Remove directly in the Download method.

Curious to see an opinion and whether this was an oversight. The current pattern makes it really hard to use hc-install in an environment that repeatedly tries to install something e.g. terraform.

@hashicorp-cla
Copy link

hashicorp-cla commented Nov 10, 2022

CLA assistant check
All committers have signed the CLA.

@alenkacz
Copy link
Contributor Author

@radeksimko would you have time to take a look? 🙏

@radeksimko radeksimko self-requested a review November 14, 2022 10:31
@alenkacz
Copy link
Contributor Author

2022/11/14 10:19:41 install_go_version.go:36: download of go "1.18" finished
2022/11/14 10:19:41 go_build.go:55: executing go1.18 ["build" "-o" "C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\hc-install-vault-3b59ff0bb9c90fc06608ae50d64ecb9fe6b1cf4c2422214891\\vault.exe"] in "C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\hc-install-build-vault3473015084"
2022/11/14 10:27:02 git_revision.go:171: building of vault finished
panic: test timed out after 20m0s

seems unrelated to my PR but 🤔 any guidance?

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alenkacz 👋🏻
The test failure is indeed unrelated to your PR. I have already raised a separate PR to address the failure in #75

The proposed code changes look generally reasonable to me and I'd be happy to merge the PR once we get the tests green, i.e. hopefully once we merge #75

I just left you some minor in-line comments, which aren't blocking.


For context, there was no strong reason for omitting these paths from the cleanup, as far as I remember it was mostly pragmatic decision, keeping in mind that

  1. Disk space available in CI systems where this library is typically used usually fit the typical number of versions downloaded. Terraform archives in particular hover around 20-30MBs each, unless you're dealing with very old <=0.6.16 versions which used to bundle providers, those would be ~120MB. GitHub Actions workflows get 14GB of disk space by default, which should be enough for plenty of versions, probably more than we ever released 😅 but of course not everyone uses GHA and you can have less disk space available in your own CI environment, so YMMV.
  2. We purposefully use $TMPDIR and equivalent on other OSes, such that the OS can easily do the clean up. Of course the default cleanup behaviour may not be convenient for everyone and may not always be easily tweak-able.

All in all - this is a very reasonable change and thank you for the contribution!

checkpoint/latest_version.go Outdated Show resolved Hide resolved
releases/exact_version.go Outdated Show resolved Hide resolved
releases/latest_version.go Outdated Show resolved Hide resolved
Currently, this file is written to tmp root with a random name and is
not cleaned.
@alenkacz
Copy link
Contributor Author

@radeksimko thank you for the thoughtful review.

Oh yeah, that makes sense with the CI. Our use case is actually a bit different and we don't use it in CI but instead inside a long living container where we need to call it repeatedly and these zips not being cleaned up are taking a lot of space

@alenkacz
Copy link
Contributor Author

@radeksimko any update on this? Looks like this time I was luckier and even CI is green :D

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.

None yet

3 participants