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

Vendor in latest containers/common with default capabilities #16733

Merged
merged 1 commit into from
Dec 16, 2022
Merged

Vendor in latest containers/common with default capabilities #16733

merged 1 commit into from
Dec 16, 2022

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Dec 4, 2022 •
edited
Loading

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

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note-none and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Dec 4, 2022
@rhatdan rhatdan mentioned this pull request Dec 5, 2022
Merged
@rhatdan rhatdan force-pushed the caps branch 3 times, most recently from a349e6f to d307bca Compare December 7, 2022 00:51
@rhatdan rhatdan changed the title [WIP] Vendor in latest containers/common with default capabilities Vendor in latest containers/common with default capabilities Dec 7, 2022
@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 Dec 7, 2022
Luap99
Luap99 reviewed Dec 7, 2022
View reviewed changes
test/e2e/run_security_labels_test.go Outdated
@@ -58,7 +58,7 @@ var _ = Describe("Podman generate kube", func() {

ctr := inspect.InspectContainerToJSON()
caps := strings.Join(ctr[0].EffectiveCaps, ",")
Expect(caps).To(Not(Equal("CAP_SYS_ADMIN")))
Expect(caps).To(Not(Equal("CAP_SYSADMIN")))
Copy link
Member

Choose a reason for hiding this comment

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

The cap is called SYS_ADMIN so this looks wrong, since you match with Not (Equal()) you turn this into a noop.

@Luap99
Copy link
Member

Luap99 commented Dec 9, 2022

Can you get this working? We need this to vendor a new c/common version.

@Luap99 Luap99 mentioned this pull request Dec 9, 2022
Closed
@Luap99
Copy link
Member

Luap99 commented Dec 9, 2022

@rhatdan To fix the build issue just append .Path() to the netns var in the UnmountNS() call

@rhatdan
Copy link
Member Author

rhatdan commented Dec 9, 2022

@Luap99 hopefully fixed, at least builds now.

Luap99
Luap99 reviewed Dec 10, 2022
View reviewed changes
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.

LGTM

@TomSweeneyRedHat
Copy link
Member

builds aren't happy @rhatdan

@flouthoc flouthoc mentioned this pull request Dec 11, 2022
Merged
@rhatdan rhatdan force-pushed the caps branch 5 times, most recently from fdde75c to 8521054 Compare December 15, 2022 17:07
@rhatdan rhatdan added the bloat_approved Approve a PR in which binary file size grows by over 50k label Dec 15, 2022
@rhatdan
Vendor in latest containers/common with default capabilities
3718ac8 
Also update vendor of containers/storage and image

Cleanup display of added/dropped capabilties as well

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

rhatdan commented Dec 16, 2022

@giuseppe @vrothberg @flouthoc @mheon This one is finally ready to merge.

@containers/podman-maintainers PTAL

@mheon
Copy link
Member

mheon commented Dec 16, 2022

LGTM

flouthoc
flouthoc approved these changes Dec 16, 2022
View reviewed changes
Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM

@flouthoc flouthoc requested a review from giuseppe December 16, 2022 08:38
vrothberg
vrothberg approved these changes Dec 16, 2022
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 16, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, rhatdan, vrothberg

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:
  • OWNERS [flouthoc,rhatdan,vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 18f1a80 into containers:main Dec 16, 2022
@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 17, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 left review comments

@vrothberg vrothberg vrothberg approved these changes

@flouthoc flouthoc flouthoc approved these changes

@giuseppe giuseppe Awaiting requested review from giuseppe

Assignees

@vrothberg vrothberg

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bloat_approved Approve a PR in which binary file size grows by over 50k 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. release-note-none
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@rhatdan @Luap99 @TomSweeneyRedHat @mheon @flouthoc @vrothberg @openshift-merge-robot