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.

Sign up for GitHub

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

Jump to bottom

Cleanup display of trust with transports #13247

Merged
merged 1 commit into from
Feb 23, 2022
Merged

Cleanup display of trust with transports #13247

merged 1 commit into from
Feb 23, 2022

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Feb 16, 2022

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

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 16, 2022

[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 Feb 16, 2022
@mheon
Copy link
Member

mheon commented Feb 16, 2022

LGTM but you have whitespace issues

@rhatdan rhatdan force-pushed the trust branch 4 times, most recently from 126cb05 to ea7ad3e Compare February 17, 2022 19:46
TomSweeneyRedHat
TomSweeneyRedHat reviewed Feb 18, 2022
View reviewed changes
docs/source/markdown/podman-image-trust.1.md Outdated

#### **--raw**
Output trust policy file as raw JSON
#### show OPTIONS
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
#### show OPTIONS
### show OPTIONS

Copy link
Member

Choose a reason for hiding this comment

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

Two was too big and conflicting with OPTIONS, but Four is too small and conflicts with the options themselves. I'd go for in between with three hashes for this and set OPTIONS

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

TomSweeneyRedHat
TomSweeneyRedHat reviewed Feb 18, 2022
View reviewed changes
docs/source/markdown/podman-image-trust.1.md Outdated
@@ -40,6 +40,8 @@ Trust may be updated using the command **podman image trust set** for an existin
#### **--help**, **-h**
Print usage statement.

#### set OPTIONS
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
#### set OPTIONS
### set OPTIONS

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@TomSweeneyRedHat
Copy link
Member

Couple of nits, plus the whitespace issues Matt noted.

@rhatdan rhatdan force-pushed the trust branch 3 times, most recently from d40b551 to 6608f7f Compare February 21, 2022 21:32
@rhatdan
Copy link
Member Author

rhatdan commented Feb 22, 2022

@containers/podman-maintainers PTAL
This looks like it is ready to go in.

@mheon
Copy link
Member

mheon commented Feb 22, 2022

You have an open comment from Tom

umohnani8
umohnani8 reviewed Feb 22, 2022
View reviewed changes
docs/source/markdown/podman-image-trust.1.md Outdated

#### **--json**, **-j**
Output trust as JSON for machine parsing

#### **--noheading**, **-n**
Omit the table headings from the tust listings
Copy link
Member

Choose a reason for hiding this comment

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

typo: s/tust/trust

@edsantiago
Copy link
Member

Is it too late for a friendly suggestion?
pr13247.diff.txt

@rhatdan
Copy link
Member Author

rhatdan commented Feb 22, 2022

Applied

@edsantiago
Copy link
Member

Thanks. LGTM.

@rhatdan rhatdan added lgtm Indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 22, 2022
@TomSweeneyRedHat
Copy link
Member

LGTM

@TomSweeneyRedHat
Copy link
Member

/lgtm

@umohnani8 umohnani8 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 23, 2022
@openshift-merge-robot openshift-merge-robot merged commit ccb96a2 into containers:main Feb 23, 2022
@edsantiago edsantiago mentioned this pull request Feb 24, 2022
Closed
@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 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@umohnani8 umohnani8 umohnani8 left review comments

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

Assignees

@TomSweeneyRedHat TomSweeneyRedHat

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rhatdan @mheon @TomSweeneyRedHat @edsantiago @umohnani8 @openshift-merge-robot