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

Fix Podman image trust tests #12060

Merged
merged 3 commits into from
Nov 3, 2021
Merged

Fix Podman image trust tests #12060

merged 3 commits into from
Nov 3, 2021

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Oct 21, 2021 •
edited
Loading

What this PR does / why we need it:

Fix podman image trust not to assume a specific system-wide configuration, and make the tests a bit stricter. Relevant for #11795 .

How to verify it

podman image trust tests succeed both on F35 (which has changed the system-wide policy.json) and in all other environments.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

See #11795 (comment) for more discussion.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 21, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 21, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtrmac

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 21, 2021
@mtrmac mtrmac mentioned this pull request Oct 21, 2021
Merged
@mtrmac mtrmac force-pushed the podman-trust-show-f35 branch 3 times, most recently from 85067d0 to 8b6b4fb Compare October 25, 2021 08:07
@mtrmac
Copy link
Collaborator Author

mtrmac commented Oct 25, 2021

The podman image trust parts seem to work now; up to @cevich whether this should be included via some other PR, or merged stand-alone to the main branch.

mtrmac added 3 commits October 25, 2021 18:02
@mtrmac
Use INTEGRATION_ROOT instead of current directory
c872788 
Should not change behavior, just to set a consistent
precedent for code introduced in future commits.

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac
Tighten the expected output of the "podman image trust show" test
53ff492 
... to include all fields.

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac
Fix tests of podman image trust --raw and --json
adee084 
Instead using the OS-wide system default policy, use
the one in this repo, and adjust the expected results
(as well as making the test stricter).

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac mtrmac force-pushed the podman-trust-show-f35 branch from 8b6b4fb to adee084 Compare October 25, 2021 16:04
@mtrmac
Copy link
Collaborator Author

mtrmac commented Oct 25, 2021

Rebased on top of main to contain only the podman image trust parts, ready for review.

@mtrmac mtrmac marked this pull request as ready for review October 25, 2021 16:05
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 25, 2021
mtrmac
mtrmac commented Oct 25, 2021
View reviewed changes
Copy link
Collaborator Author

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

FWIW some surprising, probably unwanted, behavior I have noticed.

test/e2e/trust_test.go
})

It("podman image trust show --raw", func() {
session := podmanTest.Podman([]string{"image", "trust", "show", "--raw"})
session := podmanTest.Podman([]string{"image", "trust", "show", "--policypath", filepath.Join(INTEGRATION_ROOT, "test/policy.json"), "--raw"})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

podman image trust show $path/test.policy.json --raw (missing the --policypath option, i.e. with an extra argument) does not fail.

test/e2e/trust_test.go
"name": "docker.io/library/hello-world",
"repo_name": "docker.io/library/hello-world",
"sigstore": "",
"transport": "",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that transport is always empty.

@rhatdan
Copy link
Member

rhatdan commented Oct 25, 2021

LGTM

@cevich
Copy link
Member

cevich commented Oct 26, 2021 •
edited
Loading

@mtrmac would you mind fixing up the PR description...I think this should get rebased against main and merged in (assuming code-ready).

Edit: Nevermind, you already rebased, I needed to refresh the page.

@cevich
Copy link
Member

cevich commented Nov 2, 2021

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

rhatdan commented Nov 3, 2021

/hold cancel

@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 Nov 3, 2021
@openshift-merge-robot openshift-merge-robot merged commit 8fdde67 into containers:main Nov 3, 2021
@mtrmac mtrmac deleted the podman-trust-show-f35 branch November 3, 2021 21:45
@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.
Reviewers
No reviews
Assignees

@cevich cevich

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.
4 participants
@mtrmac @rhatdan @cevich @openshift-merge-robot