-
Notifications
You must be signed in to change notification settings - Fork 144
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
feat: Add ability to customize / upgrade terraform version #17
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the quick turnaround !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really agree with this. The Dockerfile was already parameterized, why do we need to add a layer of indirection via Terraform?
@morgante - This change allows people to customize the terraform version without having to fork the actual module to make there own copy, they can still use the CFT module from the registry / Github. Let me know if you think there is a better solution to allow this? It's very common for teams to use a specific version of terraform. |
Can't you simply override the env variables on the build to handle that, instead of introducing templating? |
So you would prefer we passed through the variables to the |
Yeah I think that solution might be preferable. Templating locks this into Terraform, while the env-approach lets you easily copy the Dockerfile elsewhere if need be. |
7bea922
to
01ce76c
Compare
I have updated now so that Cloud Build or Dockerfile can be used independently if people want and still paramaterize the versions using terraform variables. Let me know what you think @morgante |
0779968
to
134225b
Compare
134225b
to
e7a48e6
Compare
@morgante - anything holding this back from merging now? |
Solution to #16