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] Update podman.1.md, image-(diff, mount, exists) & images #10536

Closed
wants to merge 1 commit into from
Closed

[CI:DOCS] Update podman.1.md, image-(diff, mount, exists) & images #10536

wants to merge 1 commit into from

Conversation

infiniteregrets
Copy link

@infiniteregrets infiniteregrets commented Jun 2, 2021

Hi, so I just tried to fix the podman.1.md page according to the https://github.com/containers/podman/edit/master/docs/MANPAGE_SYNTAX.md. I was able to do the same for some other pages as well, but before I push the changes I would like to know if this page is okay. I might have made some mistakes, so please do lmk! and I will fix them asap.

I have a few questions regarding the page:

  • I could not find any option named --runtime-flag on the version of podman I am using which is 2.0.6. Was this recently introduced?
podman --help | grep -nA 2 -nB 2 runtime
77-      --root string               Path to the root directory in which data, including images, is stored
78-      --runroot string            Path to the 'run directory' where all state information is stored
79:      --runtime string            Path to the OCI-compatible binary used to run containers, default is /usr/bin/runc
80-      --storage-driver string     Select which storage driver is used to manage storage of images and containers (default is overlay)
81-      --storage-opt stringArray   Used to pass an option to the storage driver
  • For the --remote option I am not sure if there is a better way to write it, "Access Podman service will be remote".
  • Should references to man pages be hyperlinks?
  • For
## Environment Variables
 
Podman can set up environment variables from env of [engine] table in containers.conf. These variables can be overridden by passing  environment variables before the `podman` commands.

why is the word in "engine" in square brackets. what would be the correct way to put it? should i just highlight this with backticks?

Thank you so much! Please lmk if there is a better way to put things and if there any related webpages for the commands/options that could be helpful for someone.

Signed off by: Mehul Arora [email protected]

@infiniteregrets infiniteregrets changed the title [CI:DOCS] [CI:DOCS] Update podman.1.md Jun 2, 2021
docs/source/markdown/podman.1.md Show resolved Hide resolved
docs/source/markdown/podman.1.md Outdated Show resolved Hide resolved
docs/source/markdown/podman.1.md Outdated Show resolved Hide resolved
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 2, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: infiniteregrets, 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 Jun 2, 2021
@rhatdan
Copy link
Member

rhatdan commented Jun 2, 2021

Please after writing your markdown do a make docs and look at the man page.

$ make docs
$ man ./docs/build/man/podman.1 

The tables in the man page do not look good and we would need to work on go2man to fix these.

docs/source/markdown/podman.1.md Outdated Show resolved Hide resolved
docs/source/markdown/podman.1.md Outdated Show resolved Hide resolved
docs/source/markdown/podman.1.md Outdated Show resolved Hide resolved
docs/source/markdown/podman.1.md Outdated Show resolved Hide resolved
docs/source/markdown/podman.1.md Outdated Show resolved Hide resolved
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.

Nice start @infiniteregrets !

@infiniteregrets
Copy link
Author

infiniteregrets commented Jun 4, 2021

Few questions:

  • Who compiled podman-image-mount(1)? It's not mentioned in doc
  • What are the available formats for the format option in the mount command?
  • When podman is run as root, the # symbol is treated as a comment when a language(eg. bash) is specified in a fenced code block. Using \ to escape it, does not work. Any ideas on how to fix this? Or should I just not specify the language?
  • Everytime I commit something trivial, tests are run. How do I stop them from running
  • Should the referenced in SEE ALSO be in bold?

@infiniteregrets infiniteregrets changed the title [CI:DOCS] Update podman.1.md [CI:DOCS] Update podman.1.md, image-(diff, mount, exists) & images Jun 8, 2021
@infiniteregrets
Copy link
Author

I cannot find links for the mentioned pages in podman.1.md. Is this https://manpages.debian.org/unstable/podman/containers-mounts.conf.5.en.html the one users should be redirected to?

@rhatdan
Copy link
Member

rhatdan commented Jun 8, 2021

Copy link
Contributor

@Procyhon Procyhon left a comment

Choose a reason for hiding this comment

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

Maybe take a look again at the updated version of the MANPAGE_SYNTAX. I am still updating it while going through the manpages. The latest PR should show you what changed.

|--------|-------------|
| A | A file or directory was added. |
| D | A file or directory was deleted. |
| C | A file or directory was changed. |

## OPTIONS

#### **--format**
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be **--format**=*format*. Maybe adding a -f option here would be also a good idea, but i believe you do not have to do this. But it is worth mentioning.

JSON should be written in small letters, since it does not work when written in caps lock. So the old version was correct.

If no there is no input behind format the contents are simply listed below each other in the file. So it is not really true that json is the only valid option.

Copy link
Author

Choose a reason for hiding this comment

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

I think acronyms are supposed to be capitalized and if you are talking about the man pages then it does work just fine?
Screen Shot 2021-06-09 at 10 15 04 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

The bold version after an eqaul sign should be used when there is a standard input. Since --format does not have this it should be italic.

Copy link
Author

Choose a reason for hiding this comment

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

I'll fix that, can you confirm the JSON part? Because I still think acronyms should be in capitals

docs/source/markdown/podman-image-exists.1.md Outdated Show resolved Hide resolved
docs/source/markdown/podman-image-exists.1.md Outdated Show resolved Hide resolved
docs/source/markdown/podman-image-exists.1.md Outdated Show resolved Hide resolved
docs/source/markdown/podman-image-mount.1.md Outdated Show resolved Hide resolved
@@ -28,26 +33,31 @@ Mount all images.

#### **--format**=*format*

Print the mounted images in specified format (json).
Print the mounted images in specified format (`JSON`).
Copy link
Contributor

Choose a reason for hiding this comment

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

JSON should also be small.

Copy link
Member

Choose a reason for hiding this comment

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

From what I can see caps is correct.... https://www.rfc-editor.org/info/std90

Copy link
Contributor

@Procyhon Procyhon Jun 10, 2021

Choose a reason for hiding this comment

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

$ podman image diff --format JSON alpine
Error: only supported value for '--format' is 'json'
$ podman image diff --format json alpine
<works>

Writing it in caps lock does not work as you can see. I think it has to be defined what is more important, that it is written in caps like an acronym is written or that the right input for the flag is listed. I think that the later variant is more important. You can still describe it in the description of the option as JSON but if you list the argument for the flag in a table or refer to it should be small.

For the description should also be mentioned what happens if you don't right an input after the equal sign. JSON is the only xml like format that can be selected here, but if you just leave it blank the output is plain text.

Btw. I think that the flag does not even work. Input after the equal sign is simply ignored.

edit: spelling

Copy link
Author

Choose a reason for hiding this comment

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

I agree, there is something buggy about this. And the examples use diff when they should be using image diff. I remember testing the command on my machine and thought I was doing something wrong. I am going test this command more and open an issue

Copy link
Member

Choose a reason for hiding this comment

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

Only the lowercase version is accepted so we should not show this as JSON

docs/source/markdown/podman-image-mount.1.md Outdated Show resolved Hide resolved
docs/source/markdown/podman-images.1.md Outdated Show resolved Hide resolved
@rhatdan
Copy link
Member

rhatdan commented Jun 9, 2021

Can you squash all of your commits.

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.

sorry for the tardy review, it's been a week! This is shaping up nicely, I left a few more comments.

@infiniteregrets
Copy link
Author

Can you squash all of your commits.

Thanks for the suggestion! I didn't know about this. I am kinda new to this whole pull request thing and I am not sure if this is the right place to ask this but, after squashing a few commits I was trying to sign the ones I had not signed before so I found a solution on stackoverflow: git rebase -i --root --exec 'git commit --amend --no-edit --no-verify -S' and this pretty much deleted all the files from the fork I had cloned. I have been trying to find a way to just get back this pull request branch on my local setup but I am feeling a bit lost because when I clone the fork, I can't find the doc-page-1 branch. I am not sure if it would be possible to get back the commits I made before for other documents beside the ones in this PR, but I would appreciate any help

@TomSweeneyRedHat
Copy link
Member

@nalind do you have some git suggestions for @infiniteregrets ? Unfortunately you've gone into regions of git that I struggle with too.

@nalind
Copy link
Member

nalind commented Jun 10, 2021

git reflog doc-page-1 will show when the branch was changed to have a different commit as its tip. You can use git log $commitid to view the log for a given commit ID, and if you see the one you wanted, you can check out the commit as the branch by first moving the wrong one out of the way by using git branch -m doc-page-1 not-doc-page-1 and then recreating the branch with git checkout -b doc-page-1 $commitid. This might miss the defaults that you previously had for pushing from your local branch to your fork, but a manual git push -f origin doc-page-1:doc-page-1 (assuming your fork is "origin") will continue to work.

@infiniteregrets
Copy link
Author

Thanks @nalind, looks like I was able to make it work somehow. Unfortunately the changes I made in other files which weren't pushed here are lost

@infiniteregrets
Copy link
Author

infiniteregrets commented Jun 10, 2021

@TomSweeneyRedHat can you please confirm who compiled podman-image-mount(1) originally, it's not mentioned in the doc and would be nice to have it there since other docs have it

@rhatdan
Copy link
Member

rhatdan commented Jun 11, 2021

I believe it was me.

commit 6979d140f1c531fd32e885542be27407105ebf90
Author: Daniel J Walsh <[email protected]>
Date:   Tue Jul 28 10:25:14 2020 -0400

    Add podman image mount

If you do git log PATHTOSRC

It will show you the history of the source. If you go to the end, it will show you who created the file.

@TomSweeneyRedHat
Copy link
Member

@infiniteregrets Where does this one stand now? It looks like you've learned more about git than you anticipated! However, you have a white space issue somewhere that the CI doesn't like, a file that's missing, and we've one or two other PR's in flight for some of these files. Are you able to move this forward?

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

On the whole, this looks good. Some comments, please make sure options/arguments work when you change them (uppercase vs lowercase etc...)
The man pages are validated with two scripts: hack/xref-helpmsgs-manpages and hack/man-page-checker please make sure they pass. I think the extra newline after the headings causes the script to fail.


## OPTIONS

#### **--format**
#### **--format**, **-f**=*format*
Copy link
Member

Choose a reason for hiding this comment

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

-f is not valid for image diff

$ podman image diff -f json alpine
Error: unknown shorthand flag: 'f' in -f

podman-image-diff - Inspect changes on an image's filesystem

## SYNOPSIS

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 our validate script does not allow an extra newline after the heading.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does work, but for the uniformity of all manpages I'll specify in the MANPAGE_SYNTAX, that there shouldn't be a new line.

Copy link
Author

Choose a reason for hiding this comment

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

@Procyhon I am using markdownlint, which is a vscode extension and they suggest using a new line https://github.com/DavidAnson/markdownlint/blob/v0.23.1/doc/Rules.md#md022
I think it just makes the markdown source code more readable

Copy link
Author

Choose a reason for hiding this comment

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

But let me know what is the final verdict on this

**podman image mount** [*options*] [*image* ...]

**podman image mount** [*options*] name
**podman mount** [*options*] name
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct podman mount is used for containers. This page is about podman image mount.

Copy link
Contributor

@Procyhon Procyhon Jun 13, 2021

Choose a reason for hiding this comment

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

[OUTDATED]
The synopsis was correct. The input of images after the command or the options is only required when the --all option isn't used. And I believe that image is the better term 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 synopsis was correct. The input of images after the command or the options is only required when the --all option isn't used. And I believe that image is the better term here.

I would revert this comment.

I added the following definition for cases like these into the MANPAGE_SYNTAX:

Many manpages include the OPTIONS --all, -a and/or --latest, -l. In this case there is no container name or ID needed after the initial command. Because most of the other OPTIONS still need the container name or ID, it is defined that the container argument in the command should not be put in brackets. It should also be noted in the IMPORTANT section in the DESCRIPTION of the OPTION.

I believe this is a good way to work with --all, -a and --latest, -l without compromising the command structure and keeping the general idea behind it intact.


Alter the output into a different format. The only valid format for diff is `json`.
Alter the output into a different format. The only valid format for diff is `JSON`.
Copy link
Member

Choose a reason for hiding this comment

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

JSON should be lowercase. The uppercase one throws an error.

Copy link
Member

Choose a reason for hiding this comment

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

Where is the error coming from on JSON vs json? We need to stop that error, JSON is the correct capitalization.

Copy link
Author

Choose a reason for hiding this comment

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

It has to be fixed in containers/common https://github.com/containers/common/blob/6c933f226ed44491d082d6d52cbe31ae6fe0e76f/pkg/report/validate.go#L5 where it should be probably (json|JSON)

@@ -28,26 +33,31 @@ Mount all images.

#### **--format**=*format*

Print the mounted images in specified format (json).
Print the mounted images in specified format (`JSON`).
Copy link
Member

Choose a reason for hiding this comment

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

Only the lowercase version is accepted so we should not show this as JSON

docs/source/markdown/podman.1.md Outdated Show resolved Hide resolved
docs/source/markdown/podman.1.md Outdated Show resolved Hide resolved
docs/source/markdown/podman.1.md Outdated Show resolved Hide resolved
docs/source/markdown/podman.1.md Outdated Show resolved Hide resolved

## EXAMPLE

- Diff of two different python images.
Copy link
Member

Choose a reason for hiding this comment

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

podman image diff will accept only one argument so the examples are wrong.

@infiniteregrets
Copy link
Author

infiniteregrets commented Jun 12, 2021

@Luap99 I was able to fix #10649 and add the functionality to find diff between two images. Although I am done with my work but when I run make validate I end up with a git diff + make: *** [Makefile:232: gofmt] Error 1 and make lint gives me:

VERSION=1.36.0 GOBIN=/home/mehul/go/bin sh ./hack/install_golangci.sh
golangci-lint has version 1.36.0 built from 6c25d06 on 2021-01-26T13:14:05Z
Using existing ./bin/golangci-lint has version 1.36.0 built from 6c25d06 on 2021-01-26T13:14:05Z
hack/golangci-lint.sh run

Running golangci-lint for tunnel
Build Tags tunnel: apparmor,seccomp,selinux,linter,remote,remoteclient
Skipped directories tunnel: pkg/api
WARN [runner] Can't run linter goanalysis_metalinter: bodyclose: failed prerequisites: [[email protected]/containers/podman/v3/pkg/api/handlers: analysis skipped: errors in package: [/home/mehul/Documents/test/podman/pkg/api/handlers/types.go:7:2: could not import github.com/containers/common/libimage (/home/mehul/go/pkg/mod/github.com/containers/[email protected]/libimage/copier.go:16:19: could not import github.com/containers/image/v5/storage (/home/mehul/go/pkg/mod/github.com/containers/image/[email protected]/storage/storage_image.go:25:2: could not import github.com/containers/storage (/home/mehul/go/pkg/mod/github.com/containers/[email protected]/store.go:16:4: could not import github.com/containers/storage/drivers/register (/home/mehul/go/pkg/mod/github.com/containers/[email protected]/drivers/register/register_btrfs.go:7:4: could not import github.com/containers/storage/drivers/btrfs (/home/mehul/go/pkg/mod/github.com/containers/[email protected]/drivers/btrfs/btrfs.go:15:8: could not import C (cgo preprocessing failed))))))]] 
WARN [runner] Can't run linter unused: buildir: failed to load package btrfs: could not load export data: no export data for "github.com/containers/storage/drivers/btrfs" 
ERRO Running error: buildir: failed to load package btrfs: could not load export data: no export data for "github.com/containers/storage/drivers/btrfs" 
ERRO Timeout exceeded: try increasing it by passing --timeout option 
make: *** [Makefile:227: golangci-lint] Error 4

Just feeling a bit overwhelmed looking at this? Any ideas on what I could be possibly doing wrong?
And not completely sure, but I have to add the tests for image diff here: https://github.com/containers/podman/blob/master/test/e2e/diff_test.go right?

I also changed the regular expression to accept JSON as well, is that something desirable? Or should I just revert it back to normal?

Also, I am really sorry for not testing everything thoroughly. Next time I'll test as I go! Just had a hard time configuring my environment, finally switched to a fedora VM and everything works smoothly.

Should I just open a pull request and ask for help there?

@infiniteregrets
Copy link
Author

Thanks @TomSweeneyRedHat, I will be able to move this forward once the image diff thing is fixed

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 13, 2021

@infiniteregrets: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2021
@Luap99
Copy link
Member

Luap99 commented Jun 13, 2021

@Luap99 I was able to fix #10649 and add the functionality to find diff between two images. Although I am done with my work but when I run make validate I end up with a git diff + make: *** [Makefile:232: gofmt] Error 1 and make lint gives me:

VERSION=1.36.0 GOBIN=/home/mehul/go/bin sh ./hack/install_golangci.sh
golangci-lint has version 1.36.0 built from 6c25d06 on 2021-01-26T13:14:05Z
Using existing ./bin/golangci-lint has version 1.36.0 built from 6c25d06 on 2021-01-26T13:14:05Z
hack/golangci-lint.sh run

Running golangci-lint for tunnel
Build Tags tunnel: apparmor,seccomp,selinux,linter,remote,remoteclient
Skipped directories tunnel: pkg/api
WARN [runner] Can't run linter goanalysis_metalinter: bodyclose: failed prerequisites: [[email protected]/containers/podman/v3/pkg/api/handlers: analysis skipped: errors in package: [/home/mehul/Documents/test/podman/pkg/api/handlers/types.go:7:2: could not import github.com/containers/common/libimage (/home/mehul/go/pkg/mod/github.com/containers/[email protected]/libimage/copier.go:16:19: could not import github.com/containers/image/v5/storage (/home/mehul/go/pkg/mod/github.com/containers/image/[email protected]/storage/storage_image.go:25:2: could not import github.com/containers/storage (/home/mehul/go/pkg/mod/github.com/containers/[email protected]/store.go:16:4: could not import github.com/containers/storage/drivers/register (/home/mehul/go/pkg/mod/github.com/containers/[email protected]/drivers/register/register_btrfs.go:7:4: could not import github.com/containers/storage/drivers/btrfs (/home/mehul/go/pkg/mod/github.com/containers/[email protected]/drivers/btrfs/btrfs.go:15:8: could not import C (cgo preprocessing failed))))))]] 
WARN [runner] Can't run linter unused: buildir: failed to load package btrfs: could not load export data: no export data for "github.com/containers/storage/drivers/btrfs" 
ERRO Running error: buildir: failed to load package btrfs: could not load export data: no export data for "github.com/containers/storage/drivers/btrfs" 
ERRO Timeout exceeded: try increasing it by passing --timeout option 
make: *** [Makefile:227: golangci-lint] Error 4

Just feeling a bit overwhelmed looking at this? Any ideas on what I could be possibly doing wrong?

Can you compile with make binaries? You could try to run make vendor and try again after that. If you cannot get this to work locally you could also just push and let the CI do its job.

And not completely sure, but I have to add the tests for image diff here: https://github.com/containers/podman/blob/master/test/e2e/diff_test.go right?

Yes

I also changed the regular expression to accept JSON as well, is that something desirable? Or should I just revert it back to normal?

I think it's good to accept both.

Also, I am really sorry for not testing everything thoroughly. Next time I'll test as I go! Just had a hard time configuring my environment, finally switched to a fedora VM and everything works smoothly.

Don't worry, every beginning is difficult. Thanks for your work.

Should I just open a pull request and ask for help there?

Yes, that would be good.

@infiniteregrets
Copy link
Author

@Luap99 make binaries and make vendor works just fine. I don't know why make validate and make lint don't work. I have opened a pull request and let's see if it passes the tests.
Thanks!

@vrothberg
Copy link
Member

@infiniteregrets, can you rebase?

@rhatdan rhatdan closed this Jun 30, 2021
@rhatdan rhatdan deleted the branch containers:master June 30, 2021 15:09
@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 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 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. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants