-
-
Notifications
You must be signed in to change notification settings - Fork 692
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 support for additional Docker options and pre pip/npm commands #360
Closed
AllanBenson001
wants to merge
6
commits into
terraform-aws-modules:master
from
AllanBenson001:docker-opts-and-pre-commands
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
d289624
Add docker_additional_options and pre_package_commands
AllanBenson001 ccf12bb
TF Format
AllanBenson001 d559578
Update docs
AllanBenson001 c5991cc
Merge branch 'master' into docker-opts-and-pre-commands
AllanBenson001 6c707f6
Merge branch 'master' into docker-opts-and-pre-commands
AllanBenson001 0984afa
Fix issues introduced by #358/#359
AllanBenson001 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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'm not sure about this. I think it is a pretty strong anti-pattern for a terraform plan to pass, that would fail in the apply phase. Every plan should expose issues with the pipeline, to catch issues as inputs change (e.g. the runtime was python3.8 but was updated to python3.9). If the system where I'm running the plan cannot actually build the package in the apply phase, that should be known before running the apply. That's true whether using docker or not.
However, I think we can make it flexible to the user. But instead of checking docker, check whether the user has explicitly told us to skip this check, using a new argument.
where
runtime_required
comes from thesource_path
map. E.g.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.
Thanks for the review. After merging the latest changes into my branch, a lot of the build-package tests failed that were previously succeeding, this is because I don't have things like nodejs installed. The check above checks the runtime on the host machine, this is pretty pointless when you're building your package in a docker container (what would be the point in docker if you already had the correct runtime installed locally?). The change you've suggested would work, but I consider this to be a breaking change as anyone in the same situation as me would have to add that setting to get previously working scripts to succeed. If
runtime_required
defaulted to false (or rename tocheck_runtime
ordo_runtime_check
that you have to set to true if you want to do the check), that would then not constitute a breaking change but it would almost render the runtime check useless so I'm not sure that's correct either (the runtime check is a good thing to have).I fully agree that you don't want your plan to pass when the apply phase would fail, but neither do you want it to fail when the apply would have succeeded.
I'll await further feedback before proceeding.
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.
The problem I see, is that relies on information not available to terraform at plan time. Only the user knows that the apply would succeed, because the user is proactively managing their pipeline separate from their local environment, and ensuring that the runtime is present. That feels... so very fragile. Change the runtime in the code, plan still claims success, merge, apply fails. Oh, update pipeline with new runtime, re-run job. That scenario is exactly what the test was meant to catch.
Ok, that is news to me, and certainly a valid concern. I was under the impression this packaging script ran inside the docker container, where the runtime would be available, and of course the user should be specifying a container that has the correct runtime. That deserves some more investigation...
(Fwiw, I'm just another user/contributor, not a maintainer, but since I proposed the change that caused your breakage, I feel responsible for providing context around why the change was made, and trying to help find an acceptable way forward for both concerns.)
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 appreciate your comments
Unless overridden, the runtime variable is used to choose the image that's used, see here, so it should be pretty safe.
I haven't contributed here before so I don't know if there's more that I need to do to get this PR approved. The change mentioned above is obviously mixed in with other changes which might not be approved, so it might be be necessary to fix this in a separate PR because as it stands, I'm not able to use the latest version of this module due to this error (and I've just seen that I'm not the only one - #361).
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.
Ok. That does still run into problems when the user specifies
image
separately, and forgets to update the image when they update the runtime. But, at least it makes sense for the default case. And perhaps for now the relationship between a custom image and the runtime and plan vs apply-time failures can be covered in the README somewhere.I do think it would be better to handle fixing the breakage in a separate PR. I can take a crack at that, or defer to you since the approach using
query.docker
is your idea?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'll go ahead and cherry-pick your commit, and open the PR...
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.
@antonbabenko @AllanBenson001 See #362