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

Fix ENTRYPOINT documentation, drop others. #2138

Merged
merged 1 commit into from Oct 30, 2023
Merged

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Oct 24, 2023

ENTRYPOINT was incorrectly documented to be set to / (which doesn't even make sense).

Stop mentioning PATH and WORKDIR in the top-level README, typical users of the container shouldn't need to care, and it's already somewhat implied by "built using the latest Fedora".

Fixes #2134.

We should also update the READMEs in some https://quay.io/organization/skopeo repos and https://quay.io/repository/containers/skopeo ; @TomSweeneyRedHat do you know who has write access to that?

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

The PATH in the container images is set to the default PATH provided by Fedora. Also, the
ENTRYPOINT and the WORKDIR variables are not set within these container images, as such they
default to `/`.
ENTRYPOINT of the container is set to execute the `skopeo` binary.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ENTRYPOINT of the container is set to execute the `skopeo` binary.
The ENTRYPOINT of the container is set to execute the `skopeo` binary.

Or, and I'm on the fence about noting the default values or not noting them here as they can be found elsewhere

Suggested change
ENTRYPOINT of the container is set to execute the `skopeo` binary.
The PATH in the container images is set to the default PATH provided by Fedora, and as it is not set, the WORKDIR variable defaults to `/`. The ENTRYPOINT of the container is set to execute the `skopeo` binary.

Your call on which to use. I'd go with the lengthier one, but it's a coin flip to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, fixed the first part.

As for PATH/WORKDIR, the way I think about this (and the way the README documents usage), users are guided to say something like podman run …/skopeo…:latest copy, and not actually do anything inside the container. So PATH/WORKDIR should not matter for such usage. Of course users can execute the container with --entrypoint=/bin/sh… and the Containerfiles do make some effort to make container-storage: work within the container itself.

So it seems to me the question is whether this document is a short introduction to using the image, or whether it is documenting our “API” commitment to have PATH and WORKDIR set that way. I could see an argument for either, I very weakly prefer brevity.

ENTRYPOINT was incorrectly documented to be set to /
(which doesn't even make sense).

Stop mentioning PATH and WORKDIR in the top-level README,
typical users of the container shouldn't need to care,
and it's already somewhat implied by "built using the latest Fedora".

Fixes containers#2134.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

LGTM
@mtrmac pushed me on the brevity side of the fence! 😃

@mtrmac mtrmac merged commit ae60fd8 into containers:main Oct 30, 2023
24 checks passed
@mtrmac mtrmac deleted the entrypoint branch October 30, 2023 17:39
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

misleading ENTRYPOINT info in README
3 participants