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

feat: use CMD/ENTRYPOINT from source image by default #167

Merged
merged 6 commits into from
Dec 22, 2023

Conversation

gardar
Copy link
Contributor

@gardar gardar commented Dec 7, 2023

With this change the images built will by default get the CMD and ENTRYPOINT from the source image if not overwritten by a user in the changes configuration option.
This replicates the behavior when building images from a Dockerfile.

I found numerous complaints which all seem to be related to the behaviour of the docker builder overwriting the CMD / ENTRYPOINT which this change should solve. (Kind of surprising to see that this hasn't been fixed yet 😵‍)

Closes #158
Closes #132
Closes #13
Closes #9
Closes hashicorp/packer#4914

@gardar gardar requested a review from a team as a code owner December 7, 2023 21:29
@hashicorp-cla
Copy link

hashicorp-cla commented Dec 7, 2023

CLA assistant check
All committers have signed the CLA.

Signed-off-by: gardar <[email protected]>
@gardar
Copy link
Contributor Author

gardar commented Dec 13, 2023

ping @nywilken @SwampDragons

@jkstpierre
Copy link

Can we get this approved and merged? I would love to start using this.

Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

Hi @gardar,

Thanks for the patch, this looks good to me!
I'll merge this PR and release the plugin today with that fix in.

@lbajolet-hashicorp
Copy link
Contributor

FYI: the new step wasn't goimport'd so I took the liberty to run it on the file and add that commit on top of this PR.
I'll wait until tests go green, and I'll merge.

@lbajolet-hashicorp lbajolet-hashicorp merged commit 92d8220 into hashicorp:main Dec 22, 2023
11 checks passed
@gardar
Copy link
Contributor Author

gardar commented Dec 22, 2023

FYI: the new step wasn't goimport'd so I took the liberty to run it on the file and add that commit on top of this PR. I'll wait until tests go green, and I'll merge.

Thanks!
Any chance you'll also publish a new release with the fix? Edit nvm, see you are already on top of it #168

}
if !hasEntrypoint && defaultEntrypoint != "" {
config.Changes = append(config.Changes, "ENTRYPOINT "+defaultEntrypoint)
}

Choose a reason for hiding this comment

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

This change broke our builds because if both values are defined they are connected. We only defined ENTRYPOINT and got the CMD from the base image which then was used as default arguments in our entrypoint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants