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

[CI:DOCS] Add information about .containerignore to podman build man page #11898

Merged
merged 1 commit into from
Oct 14, 2021

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Oct 8, 2021

Cleanup some other errors in the podman build man page.

Also slip a link between .dockerignore and containerignore.5 man page

Signed-off-by: Daniel J Walsh [email protected]

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 8, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 8, 2021
The .containerignore and .dockerignore files have the exact same syntax, if both
are in the same directory podman build will using .containerignore.

Users can specify a series of Unix shell globals in a .containerignore file to
identify files/directories to exclude.
Copy link
Contributor

Choose a reason for hiding this comment

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

will BE using? Or will use?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks.
use.

@@ -984,7 +988,7 @@ mechanism:

Exclude all doc files except Help.doc from the image.

This functionality is compatible with the handling of .dockerignore files
This functionality is compatible with the handling of .containerignore files
described here:

Copy link
Contributor

Choose a reason for hiding this comment

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

The following link to the docker website for the description of .dockerignore can be replaced with a link to the description of .containerignore - e.g. https://github.com/containers/buildah/blob/main/docs/containerignore.5.md .

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point.

Copy link
Contributor

Choose a reason for hiding this comment

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

This changeset in its completeness states explicitly, that .dockerignore and .containerignore have the same syntax, but it does not say, that the semantics is the same. Nevertheless the equivalent semantics is implied by the context.

The above paragraph states that .containerignore and .docherignore are compatible. This more or less means that the files are interchangeable. The end of the man-page has a SEE ALSO section, pointing to .containerignore(5), which I guess rendered in HTML will contain the right hyperlink.

I propose deleting the next link, as it is included by the SEE ALSO section behind .containerignore(5) and is therefore supposed to be hyperlinked.

I propose deleting the above paragraph, as it should be clear by now, that .containerignore and .dockerignore are interchangeable.

@rhatdan rhatdan force-pushed the docs branch 2 times, most recently from 323f75f to 52d1113 Compare October 8, 2021 18:38
docs/source/markdown/podman-build.1.md Outdated Show resolved Hide resolved
docs/source/markdown/podman-build.1.md Outdated Show resolved Hide resolved
Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

Plus the aforementioned typo

Podman uses the content to exclude files and directories from the context
directory, when executing COPY and ADD directives in the
Containerfile/Dockerfile

Users can specify a series of Unix shell globals in a .dockerignore file to
The .containerignore and .dockerignore files have the same syntax; if both
are in the context directory, podman build will use .containerignore.
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: '...will use only .containerignore` (boldface mine for emphasis; not needed in actual document). Without 'only', it is ambiguous whether the sentence means "will use .containerignore in addition to .dockerignore".

@@ -1009,10 +1013,10 @@ If you are using `useradd` within your build script, you should pass the
useradd to stop creating the lastlog file.

## SEE ALSO
podman(1), buildah(1), containers-certs.d(5), containers-registries.conf(5), crun(8), runc(8), useradd(8), podman-ps(1), podman-rm(1)
podman(1), buildah(1), containers-certs.d(5), containers-registries.conf(5), crun(8), runc(8), useradd(8), podman-ps(1), podman-rm(1), Containerfile(5), .containerignore(5)
Copy link
Member

Choose a reason for hiding this comment

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

I think (but am not sure) this should be containerignore(5), without the dot?

Cleanup some other errors in the podman build man page.

Also slip a link between .dockerignore and containerignore.5 man page

Signed-off-by: Daniel J Walsh <[email protected]>
@jwhonce
Copy link
Member

jwhonce commented Oct 13, 2021

LGTM


$ podman build -t imageName .

$ podman build --tls-verify=true -t imageName -f Dockerfile.simple .
$ podman build --tls-verify=true -t imageName -f Containrfile.simple .
Copy link
Member

Choose a reason for hiding this comment

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

Still typo :-(

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's please fix this before merge.

The .containerignore and .dockerignore files use the same syntax; if both
are in the context directory, podman build will only use .containerignore.

Users can specify a series of Unix shell globals in a .containerignore file to
Copy link
Member

Choose a reason for hiding this comment

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

Ugh. This is not your change, looks like it already existed, but this should be globs, not globals.

Copy link
Member

Choose a reason for hiding this comment

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

Ditto fix now plz.

@edsantiago
Copy link
Member

Two nits, neither one showstoppers, and I know the CI gauntlet is brutal these days, so I'll let you choose whether to merge or fix.

/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 Oct 13, 2021
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2021
@TomSweeneyRedHat
Copy link
Member

IDK why this didn't merge for @edsantiago , but the two nits he found should be picked before merging.

@rhatdan
Copy link
Member Author

rhatdan commented Oct 14, 2021

/hold cancel
I will fix them in the codespell PR I am about to open.

@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 Oct 14, 2021
@openshift-merge-robot openshift-merge-robot merged commit c19f257 into containers:main Oct 14, 2021
@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.

7 participants