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

cli/command/container/opts: make entrypoint json array avare #5839

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lalyos
Copy link
Contributor

@lalyos lalyos commented Feb 18, 2025

- What I did

Made the docker run --entrypoint handle entrypoint string the same way as:

  • Dockerfile's ENTRYPOINT
  • docker commit --change='ENTRYPOINT ["myEntryPoint.sh"]'

- How I did it

The enrtypoint option was already defined as strslice, so I just used its UnmarshalJSON method.

- How to verify it

Test usecases are written for new format. Or for an e2e test you can check binaries, I built from this pr

curl -Lo /tmp/docker-hack https://github.com/lalyos/cli-1/releases/download/hack/docker-$(uname)
chmod +x /tmp/docker-hack
alias docker=/tmp/docker-hack

docker run --entrypoint '["sh","-c","echo look ma entrypoint as an array"]' alpine

- Human readable description for the release notes

Docker run --entrypoint option can handle the same json array format as ENTRYPOINT in Dockerfile

closes #4870

…ax is correct

entrypoint is already defined as strslice why not use its unmarshal method

Signed-off-by: Lalyos Papp <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Feb 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.78%. Comparing base (88a019a) to head (305c048).
Report is 48 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5839      +/-   ##
==========================================
- Coverage   59.18%   58.78%   -0.40%     
==========================================
  Files         352      349       -3     
  Lines       29548    29559      +11     
==========================================
- Hits        17487    17376     -111     
- Misses      11080    11199     +119     
- Partials      981      984       +3     

@lalyos lalyos force-pushed the feat-make-entrypoint-array-avare branch from c01df63 to 305c048 Compare February 18, 2025 12:50
@thaJeztah
Copy link
Member

Thanks for contributing; this may need more eyes; I recall some discussion long time ago where it was decided to always use shell-form for this. I generally agree that being able to use "exec" form could be useful, but if we make this change we must be sure it's not ambiguous; the slightly tricky bit here is that [ and ] are valid binaries (they're aliases for test on linux, e.g. as used in scripts [ "foo" = "bar" ].

While we can try to detect if the given argument can be parsed as a valid JSON array (and fallback without), there can be some ambiguity, and we've ran into that case with the Dockerfile syntax (which also accepts either JSON (exec-form) or "shell" form, which sometimes causes hard to discover problems (e.g. ['foo', 'bar'] looks valid, but is invalid JSON as JSON doesn't allow single quotes.

@lalyos
Copy link
Contributor Author

lalyos commented Feb 18, 2025

I see, that is absolutely valid point. I was thinking about what could make this feature ambiguous: ruled out space and quotation mark but brackets are problematic.

I could add more tests, but still it might be too risky of a change. I see 2 alternatives:

  • Leave this PR as a reference so if somebody is searching, can find your concerns and if desired can create it's on patched docker cli.
  • Make it a docker cli plugin, maybe runx (like buildx ;) so an explicit change needed for this feature:
docker runx --entrypoint '["sh","-c","echo look ma entrypoint as an array"]' alpine`

I don't think many people use cli plugins, so maybe just stick with the first option.

@thaJeztah
Copy link
Member

I realise now that I had --healchcheck in mind, where it was discussed for exec-form 🙈 (sorry jumping around between too many threads 😂). In general, the same would apply though.

For --entrypoint, I think most cases would already be solvable by combining it with the CMD; when setting both, --entrypoint becomes the command to execute (.Path in the OCI spec), and CMD the arguments to pass it (.Args), which will be parsed using os.Args semantics;

docker create --entrypoint /bin/sh ubuntu -c "echo look ma entrypoint as an array"
d6741e207750af68f33c0cad6d23ea47d5ff46718c415f3f34d60fc0280cdfb1

docker inspect --format '{{json .Path}}' d6741e207750af68f33c0cad6d23ea47d5ff46718c415f3f34d60fc0280cdfb1
"/bin/sh"

docker inspect --format '{{json .Args}}' d6741e207750af68f33c0cad6d23ea47d5ff46718c415f3f34d60fc0280cdfb1
["-c","echo look ma entrypoint as an array"]

which will be parsed using os.Args semantics

ISTR that there's limitations on Windows in that respect, because Windows does not have ways to have consistent parsing; there was a long discussion around some of that, and I must admit that I forgot the intricate details; opencontainers/runtime-spec#998

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

Successfully merging this pull request may close these issues.

Accept --entrypoint=["some","argument"] for create/run command
3 participants