Review config for cmd/entrypoint after building a stage #348
Conversation
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
As mentioned in GoogleContainerTools#346, if only ENTRYPOINT is set in a stage then any CMD inherited from a parent should be cleared. If both entrypoint and cmd are set then nothing should change. I added a function and unit test to review the config file after building a stage which clears out config.Cmd if ENTRYPOINT was declared but CMD wasn't. I also added an integration test to make sure this works, which should be tested by the preexisting container-diff --metadata test.
priyawadhwa
force-pushed
the
entrypoint
branch
from
4be4f72 to
d923d5e
Compare
tejal29
suggested changes
Sep 13, 2018
pkg/executor/build.go
Outdated
| cmd := false | ||
|
|
||
| for _, c := range stage.Commands { | ||
| if c.Name() == "cmd" { |
pkg/executor/build.go
Outdated
| // reviewConfig makes sure the value of CMD is correct after building the stage | ||
| // If ENTRYPOINT was set in this stage but CMD wasn't, then CMD should be cleared out | ||
| // See Issue #346 for more info | ||
| func reviewConfig(stage config.KanikoStage, config *v1.Config) error { |
There was a problem hiding this comment.
This function does not encounter an error flow and always returns nil.
There is no need to return an error.
| FROM scratch | ||
| CMD ["mycmd"] | ||
| ENTRYPOINT ["myentrypoint"]`, | ||
| originalEntrypoint: []string{"myentrypoint"}, |
There was a problem hiding this comment.
by originalCmd, do you mean command from previous stage?
There was a problem hiding this comment.
use different values for dockerfile.CMD and dockerfile.ENTRYPOINT.
There was a problem hiding this comment.
by originalCmd, I mean the original value of Cmd in the config file (it should match the value specified in the Dockerfile)
The function reviews the Dockerfile and then clears the Cmd field if it isn't declared in the Dockerfile.
dlorenc
approved these changes
Sep 26, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
As mentioned in #346, if only ENTRYPOINT is set in a stage then any
CMD inherited from a parent should be cleared.
If both entrypoint and cmd are set then nothing should change.
I added a function and unit test to review the config file after building a stage
which clears out config.Cmd if ENTRYPOINT was declared but CMD wasn't.
I also added an integration test to make sure this works, which should
be tested by the preexisting container-diff --metadata test.