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

specgen: honor empty args for entrypoint specified as --entrypoint "" #12541

Merged

Conversation

flouthoc
Copy link
Collaborator

@flouthoc flouthoc commented Dec 8, 2021

Users should be able to override containers entrypoint using
--entrypoint "" following works fine for podman but not for podman
remote
since CreateOptions is not passed over to remote server.

Specgen ignores empty argument for entrypoint so make specgen honor
empty arguments.

Closes: #12521

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 8, 2021
@@ -409,7 +409,7 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *entities.ContainerCreateOptions
s.WorkDir = c.Workdir
if c.Entrypoint != nil {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Following block is only invoked when --entrypoint is set from CLI or remote-client

@flouthoc flouthoc force-pushed the remote_blank_entrypoint branch from 36b30b6 to abc301e Compare December 8, 2021 12:18
@flouthoc
Copy link
Collaborator Author

flouthoc commented Dec 8, 2021

@cevich I think we are reaching some account limits in CI and i am not sure what could we do here. Do you have any ideas about this ?

@@ -409,7 +409,7 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *entities.ContainerCreateOptions
s.WorkDir = c.Workdir
if c.Entrypoint != nil {
entrypoint := []string{}
if ep := *c.Entrypoint; len(ep) > 0 {
if ep := *c.Entrypoint; len(ep) >= 0 {
Copy link
Member

Choose a reason for hiding this comment

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

As written this check no longer does anything, you can just remove it.

Users should be able to override containers entrypoint using
`--entrypoint ""` following works fine for podman but not for podman
remote.

Specgen ignores empty argument for entrypoint so make specgen honor
empty arguments.

Signed-off-by: Aditya Rajan <[email protected]>
@flouthoc flouthoc force-pushed the remote_blank_entrypoint branch from abc301e to b526a0c Compare December 8, 2021 14:57
@flouthoc flouthoc requested a review from mheon December 8, 2021 14:57
@rhatdan
Copy link
Member

rhatdan commented Dec 8, 2021

/approve
/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 8, 2021
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 8, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 8, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cevich
Copy link
Member

cevich commented Dec 8, 2021

I think we are reaching some account limits in CI and i am not sure what could we do here. Do you have any ideas about this ?

It's not our CI, it's the upstream download limit that's being hit (but we're not helping) 😞 Multiple PRs are hitting this. Fix is in containers/automation_images#103 and #12547

@flouthoc
Copy link
Collaborator Author

flouthoc commented Dec 8, 2021

@edsantiago PTAL

@flouthoc
Copy link
Collaborator Author

flouthoc commented Dec 8, 2021

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 8, 2021
@openshift-merge-robot openshift-merge-robot merged commit 02eaebd into containers:main Dec 8, 2021
@edsantiago
Copy link
Member

Thank you @flouthoc !

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

podman-remote run --entrypoint "" : fails to override image entrypoint
6 participants