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

Feature: Add flag for install location (optional) #309

Merged
merged 3 commits into from
Apr 23, 2024

Conversation

ArronaxKP
Copy link
Contributor

@ArronaxKP ArronaxKP commented May 17, 2023

This change adds a new flag -i or --install that will allow you to set where the Terraform binary is downloaded/installed into. Useful for edge cases where you need to share commonly installed versions of Terraform.

This is in draft as it is built upon this PRs that should be merged first. To save headaches with merge conflicts this PR already includes that commit.

Once the above PR has been merged ping me and I can update this branch with latest master.

I have created a similar PR for tgswitch here - warrensbox/tgswitch#140

@strowi
Copy link

strowi commented Jan 3, 2024

This would be very useful for CI/CD cases where $HOME/ is not a cache-able path.

@ArronaxKP
Copy link
Contributor Author

ArronaxKP commented Jan 3, 2024

For CI/CD is what I wrote this for. It turns out we found a workaround for the issue we were having so I didn't need this in the end.

I created a test binary and published it here if you want it but I am not using it myself, I fell back to the original version by warrensbox. I think its just linux_amd64 binary only though:
https://github.com/ArronaxKP/terraform-switcher/releases/tag/tfswitch-with-install-flag

@MatrixCrawler MatrixCrawler mentioned this pull request Mar 27, 2024
@warrensbox
Copy link
Owner

We can revisit this after release v1.10

@ArronaxKP ArronaxKP force-pushed the add-install-flag branch 3 times, most recently from 8ec9f51 to 48719a5 Compare April 18, 2024 19:25
@ArronaxKP
Copy link
Contributor Author

ArronaxKP commented Apr 18, 2024

Updated with latest master changes. I have moved away from using tfswitch and tgswitch now. But I wanted to attempt to get this PR updated to as best I can. Since I haven't touched this project in over 9 months, it took a while. I "think" it now works. I tested locally quickly and the -i install works however I am using windows so not a real test. I don't have time to fully test it on all platforms.

If this is still wanted please ping me if you need anything. I will leave this PR in draft as I cannot fully test this locally. Feel free to take the code and create a new branch/PR if that is easier.

GitHub actions now pass for me:
image

Copy link
Collaborator

@yermulnik yermulnik left a comment

Choose a reason for hiding this comment

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

Thank you for re-visiting this PR.
I do hope you won't be much distracted by re-visiting it once again once the #356 is merged — the #356 brings a lot of refactoring, including the bits touched in this PR.
Thanks in advance.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/install.go Outdated Show resolved Hide resolved
www/docs/usage/commandline.md Outdated Show resolved Hide resolved
@ArronaxKP ArronaxKP force-pushed the add-install-flag branch 3 times, most recently from 26738f0 to 7272162 Compare April 19, 2024 16:28
@ArronaxKP
Copy link
Contributor Author

Resolved all comments.

If you ping me when #356 is merged. I will update this again, and re-test it.

@yermulnik
Copy link
Collaborator

Resolved all comments.

Thank you.

If you ping me when #356 is merged. I will update this again, and re-test it.

We'll try out best 🤝

@yermulnik
Copy link
Collaborator

If you ping me when #356 is merged. I will update this again, and re-test it.

We'll try out best 🤝

@ArronaxKP Pinging you

@ArronaxKP ArronaxKP force-pushed the add-install-flag branch 2 times, most recently from eabfda1 to 05ad223 Compare April 21, 2024 13:40
@ArronaxKP
Copy link
Contributor Author

As promised the PR updated with latest master. Includes the origina -i/--install flag changes.

Spotted some other warnings in other files I thought I would fix.

My github actions are successful:
image

@ArronaxKP ArronaxKP marked this pull request as ready for review April 21, 2024 13:54
lib/param_parsing/parameters.go Outdated Show resolved Hide resolved
www/docs/usage/commandline.md Outdated Show resolved Hide resolved
www/docs/usage/commandline.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@yermulnik yermulnik left a comment

Choose a reason for hiding this comment

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

LGTM with minor note below

www/docs/usage/config-files.md Outdated Show resolved Hide resolved
@yermulnik
Copy link
Collaborator

yermulnik commented Apr 22, 2024

Also please fetch master
image

@yermulnik
Copy link
Collaborator

@warrensbox @MatrixCrawler @crablab @MatthewJohn @jukie Please review.

Add the ability to pass -i or --install to change the default install location for the Terraform binaries
@ArronaxKP
Copy link
Contributor Author

Updated with latest master. My Github actions is passing as well

Copy link
Collaborator

@MatrixCrawler MatrixCrawler left a comment

Choose a reason for hiding this comment

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

LGTM

@MatrixCrawler MatrixCrawler added this to the Release 1.1.0 milestone Apr 22, 2024
@MatrixCrawler MatrixCrawler added the enhancement Refactor existing code for better performance and quality label Apr 22, 2024
@crablab
Copy link
Collaborator

crablab commented Apr 22, 2024

Will look this evening/tomorrow morning.

@yermulnik
Copy link
Collaborator

Will look this evening/tomorrow morning.

I'm giving this my "LGTM". Please merge once you get this reviewed and if you approve when you have a chance later. Thanks.

www/docs/usage/commandline.md Outdated Show resolved Hide resolved
@crablab crablab merged commit e7404d4 into warrensbox:master Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Refactor existing code for better performance and quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants