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.

Sign up for GitHub

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

Jump to bottom

setting entrypoint should clear cmd from child image #346

Closed
mathroc opened this issue Sep 10, 2018 · 2 comments
Closed

setting entrypoint should clear cmd from child image #346

mathroc opened this issue Sep 10, 2018 · 2 comments
Assignees
@priyawadhwa
Labels
kind/bug Something isn't working

Comments

@mathroc
Copy link

mathroc commented Sep 10, 2018

Actual behavior

when an entrypoint is set, the cmd from the child is not cleard

Expected behavior

the cmd should be cleared unless it was set in the same Dockerfile

see docker/docs#6142
@priyawadhwa priyawadhwa added the kind/bug Something isn't working label Sep 10, 2018
@priyawadhwa
Copy link
Collaborator

Though this is a simple concept, it may require some refactoring to implement. Right now each Dockerfile command executes independently, but this change would require ENTRYPOINT to have some knowledge of any prior CMD instructions if we wanted to implement it as a part of the command.

I think the easiest way to do this might be to add a reviewConfigFile function at the end of stage execution which would go through all commands in the stage and make sure there is a CMD instruction present. If there is an ENTRYPOINT but no CMD, then it will set configFile.Command to be empty.

@priyawadhwa priyawadhwa self-assigned this Sep 11, 2018
priyawadhwa pushed a commit to priyawadhwa/kaniko that referenced this issue Sep 11, 2018
Review config file for cmd/entyrpoint after building a stage
33fdb14 
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.
priyawadhwa pushed a commit to priyawadhwa/kaniko that referenced this issue Sep 11, 2018
Review config file for cmd/entyrpoint after building a stage
551bd4a 
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 pushed a commit to priyawadhwa/kaniko that referenced this issue Sep 11, 2018
Review config file for cmd/entyrpoint after building a stage
00f9a4a 
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 pushed a commit to priyawadhwa/kaniko that referenced this issue Sep 11, 2018
Review config for cmd/entrypoint after building a stage
5d2d282 
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 priyawadhwa mentioned this issue Sep 11, 2018
Merged
@bobcatfish
Copy link
Contributor

I think this was resolved in #348, re-open if I'm crazy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@priyawadhwa priyawadhwa

Labels
kind/bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mathroc @bobcatfish @priyawadhwa