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

Quadlet/Container: Add GroupAdd option #22601

Merged
merged 1 commit into from
May 9, 2024

Conversation

xkr47
Copy link
Contributor

@xkr47 xkr47 commented May 4, 2024

Does this PR introduce a user-facing change?

Quadlet/Container: Add GroupAdd option

I have not written anything in Go before and did not manage to run the test I added locally.

Copy link
Contributor

@ygalblum ygalblum left a comment

Choose a reason for hiding this comment

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

Thanks for the PR

The test case you've added is not executed. You need to add an entry here: https://github.com/containers/podman/blob/main/test/e2e/quadlet_test.go#L786 to pick it up

Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@xkr47
Copy link
Contributor Author

xkr47 commented May 6, 2024

Thanks for the PR

Thanks for being quick to review it proactively help me out!

The test case you've added is not executed. You need to add an entry here: https://github.com/containers/podman/blob/main/test/e2e/quadlet_test.go#L786 to pick it up

Tried to do that 🤞

@xkr47 xkr47 force-pushed the feat/quadlet-group-add branch from 0e6cf0e to 0b7cb2d Compare May 6, 2024 11:55
@ygalblum
Copy link
Contributor

ygalblum commented May 6, 2024

Please squash all commits into one (it will also solve the "Build Each Commit" test

@xkr47 xkr47 force-pushed the feat/quadlet-group-add branch 2 times, most recently from ac41fa5 to 035dfdb Compare May 6, 2024 13:09
@xkr47 xkr47 force-pushed the feat/quadlet-group-add branch from 035dfdb to ae56488 Compare May 7, 2024 06:20
Copy link
Contributor

openshift-ci bot commented May 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xkr47, ygalblum

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 May 7, 2024
@mheon
Copy link
Member

mheon commented May 7, 2024

LGTM

@xkr47
Copy link
Contributor Author

xkr47 commented May 7, 2024

Ah, right, forgot to mention, I ~copypasted the description from the podman-run manpage, so it might not be a perfect match stylewise. Let me know if you want me to work on that.

@@ -440,6 +441,18 @@ This key can be listed multiple times.
The (numeric) GID to run as inside the container. This does not need to match the GID on the host,
which can be modified with `UsersNS`, but if that is not specified, this GID is also used on the host.

### `GroupAdd=`
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid duplication, the Quadlet man page usually does not provide the entire documentation of the field. Instead, it states the equivalent argument.
So, I guess here you can keep the first sentence, maybe also the note about keep-groups and conclude with "Equivalent to the Podman --group-add option"

@ygalblum
Copy link
Contributor

ygalblum commented May 8, 2024

Ah, right, forgot to mention, I ~copypasted the description from the podman-run manpage, so it might not be a perfect match stylewise. Let me know if you want me to work on that.

Thanks for pointing this out. I've added a comment about it

@xkr47
Copy link
Contributor Author

xkr47 commented May 8, 2024

Pushed fixes and a typo in the mapping table. Let me know if you want me to squash the commits after the review.

Btw I guess it is customary not to do validation of the arguments at this stage but let podman report any errors, should there be any..?

@ygalblum
Copy link
Contributor

ygalblum commented May 8, 2024

Pushed fixes and a typo in the mapping table. Let me know if you want me to squash the commits after the review.

Btw I guess it is customary not to do validation of the arguments at this stage but let podman report any errors, should there be any..?

Yes, Quadlet leaves the argument validation to Podman. Please squash all commits

Co-authored-by: Ygal Blum <ygal.blum@gmail.com>
Signed-off-by: Jonas Berlin <xkr47@outerspace.dyndns.org>
@xkr47 xkr47 force-pushed the feat/quadlet-group-add branch from 573d6f4 to 6d1098f Compare May 8, 2024 13:00
@ygalblum
Copy link
Contributor

ygalblum commented May 9, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 9, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 63ab927 into containers:main May 9, 2024
91 checks passed
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 8, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Aug 8, 2024
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. 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants