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 man page for Containerfile and .containerignore #3549

Merged
merged 1 commit into from
Sep 29, 2021

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Sep 28, 2021

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?


@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 28, 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

@rhatdan rhatdan force-pushed the containerfile branch 2 times, most recently from a322aaf to 00cae3a Compare September 28, 2021 14:57
@rhatdan rhatdan changed the title Add man page for Containerfile and .containerignore [ci:docs] Add man page for Containerfile and .containerignore Sep 28, 2021
@rhatdan rhatdan force-pushed the containerfile branch 2 times, most recently from 74c434f to d138c25 Compare September 28, 2021 15:00
valid image. It is easy to start by pulling an image from the public
repositories.

-- **FROM** must be the first non-comment instruction in Containerfile.
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
-- **FROM** must be the first non-comment instruction in Containerfile.
-- **FROM** must be the first non-comment instruction in the Containerfile.

Copy link
Member

Choose a reason for hiding this comment

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

ARG is also permitted before the first FROM.

`FROM image@digest`

-- The **FROM** instruction sets the base image for subsequent instructions. A
valid Containerfile must have **FROM** as its first instruction. The image can be any
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
valid Containerfile must have **FROM** as its first instruction. The image can be any
valid Containerfile must have **FROM** as its first instruction. The image can be any

An ARG can be the first line too...

Copy link
Member

Choose a reason for hiding this comment

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

Are you going to adjust this?

difficult to update because it mixes with application-specific code.
The solution is to use **ONBUILD** to register instructions in advance, to
run later, during the next build stage.

Copy link
Member

Choose a reason for hiding this comment

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

Why are Arg and Onbuild not in alphanumeric order?

ARG user1
ARG buildno
...
```
Copy link
Member

Choose a reason for hiding this comment

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

I just spotted a double lined comment here and other places. Are they correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is intentional, I believe. This Containerfile is based off of the original Dockerfile man page.


```
$ podman build --build-arg HTTPS_PROXY=https://my-proxy.example.com .
```
Copy link
Member

Choose a reason for hiding this comment

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

Unless I missed it, I didn't see a note that a second FROM in a Containerfile sets the values associated with an Arg variable to nil and they must be reset if they are to be used later in the Containerfile.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added this note to ARGS section.

Feb 2015, updated by Brian Goff (cpuguy83@gmail.com) for readability
Sept 2015, updated by Sally O'Malley (somalley@redhat.com)
Oct 2016, updated by Addam Hardy (addam.hardy@gmail.com)
Aug 2021, converted Dockerfile man page to Containerfile by Dan Walsh (dwalsh@redhat.com)
Copy link
Member

Choose a reason for hiding this comment

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

It might be nice to have a Containers/Containerfiles repo with well documented Containerfiles for commona containers (nginx, onbuild examples, etc.). Not for the PR, just something we might want to consider.

Copy link
Member Author

Choose a reason for hiding this comment

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

That should be in Buildah if it was there.

@@ -0,0 +1,87 @@
% "containerignore" "28" "Sep 2021" "" "Container User Manuals"
Copy link
Member

Choose a reason for hiding this comment

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

IDK, but can we rename this to .containerignore or does that throw man pages for a loop?

Copy link
Member Author

@rhatdan rhatdan Sep 28, 2021

Choose a reason for hiding this comment

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

I am not sure. Users are not going to type that though. I did setup a link to this from .containerignore

@rhatdan rhatdan force-pushed the containerfile branch 3 times, most recently from 721c27b to 5f2d8eb Compare September 28, 2021 19:53
@rhatdan
Copy link
Member Author

rhatdan commented Sep 28, 2021

@edsantiago PTAL I need help on hack/xref-helpmsgs-manpages

@edsantiago
Copy link
Collaborator

foo.diff.txt

@edsantiago
Copy link
Collaborator

It's the add-a-test nagger: it doesn't have .gitignore in its exclusion list.

You can either add the magic no-new-tests string, or

diff --git a/tests/validate/pr-should-include-tests b/tests/validate/pr-should-include-tests
index 4cc843df..6d2a1835 100755
--- a/tests/validate/pr-should-include-tests
+++ b/tests/validate/pr-should-include-tests
@@ -45,6 +45,7 @@ fi
 filtered_changes=$(git diff --name-status $base $head |
                        awk '{print $2}'               |
                        fgrep -vx .cirrus.yml          |
+                       fgrep -vx .gitignore           |
                        fgrep -vx changelog.txt        |
                        fgrep -vx go.mod               |
                        fgrep -vx go.sum               |

@TomSweeneyRedHat
Copy link
Member

At least one small nit that needs changing and I think you need to rebase anyway. One other question.

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@ashley-cui
Copy link
Member

LGTM

@TomSweeneyRedHat
Copy link
Member

LGTM
but will open an issue to correct the verbiage that FROM must be the first line in a Conatinerfile when ARG can be too.

@TomSweeneyRedHat
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Sep 29, 2021
@openshift-merge-robot openshift-merge-robot merged commit d2ef199 into containers:main Sep 29, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 15, 2023
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.

None yet

6 participants