Skip to content

add WithAppendAdditionalGroups helper#7070

Merged
dmcgowan merged 1 commit intocontainerd:mainfrom
junnplus:additional-gids
Jun 21, 2022
Merged

add WithAppendAdditionalGroups helper#7070
dmcgowan merged 1 commit intocontainerd:mainfrom
junnplus:additional-gids

Conversation

@junnplus
Copy link
Copy Markdown
Member

@junnplus junnplus commented Jun 17, 2022

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Jun 17, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@junnplus junnplus force-pushed the additional-gids branch 3 times, most recently from f5dee99 to 81996be Compare June 17, 2022 04:52
@junnplus
Copy link
Copy Markdown
Member Author

junnplus commented Jun 17, 2022

/test all

@junnplus junnplus mentioned this pull request Jun 17, 2022
Merged
@junnplus junnplus marked this pull request as ready for review June 17, 2022 05:06
fahedouch
fahedouch reviewed Jun 17, 2022
View reviewed changes
Comment thread oci/spec_opts.go Outdated
fahedouch
fahedouch reviewed Jun 17, 2022
View reviewed changes
Comment thread oci/spec_opts.go Outdated
@junnplus junnplus force-pushed the additional-gids branch 3 times, most recently from 54d10b5 to 704d326 Compare June 17, 2022 09:20
@junnplus junnplus changed the title add WithAddedAdditionalGIDs helper add WithAppendAdditionalGroups helper Jun 17, 2022
@junnplus junnplus requested a review from fahedouch June 17, 2022 09:23
@fahedouch
Copy link
Copy Markdown
Member

fahedouch commented Jun 17, 2022

Thanks LGTM on green CI :)

@thaJeztah thaJeztah mentioned this pull request Jun 17, 2022
Closed
@junnplus
Copy link
Copy Markdown
Member Author

junnplus commented Jun 18, 2022

@AkihiroSuda PTAL, thanks!

fuweid
fuweid approved these changes Jun 18, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM on CI green

@fuweid
Copy link
Copy Markdown
Member

fuweid commented Jun 21, 2022

@junnplus would you mind to re-push it? I has rerun-ed the CI several times but it is still unhappy :(

@fuweid
Copy link
Copy Markdown
Member

fuweid commented Jun 21, 2022

Green!

@mxpv
Copy link
Copy Markdown
Member

mxpv commented Jun 21, 2022

Don't we have this one already? https://github.com/containerd/containerd/blob/main/oci/spec_opts.go#L810
Looks very similar

@junnplus
Copy link
Copy Markdown
Member Author

junnplus commented Jun 21, 2022 •
edited
Loading

@mxpv They are different, WithAdditionalGIDs set additional groups to which the user belongs, the test cases can see https://github.com/containerd/containerd/pull/7072/files .

WithAppendAdditionalGroups is to append additional groups of users.

@mxpv
Copy link
Copy Markdown
Member

mxpv commented Jun 21, 2022

oh, ok. thanks for clarification.

@mxpv
Copy link
Copy Markdown
Member

mxpv commented Jun 21, 2022

Just wondering, there are several ways how to set additionalGids field (cri, opts, and this one) and whether it worth to consolidate those somehow.

@junnplus
Copy link
Copy Markdown
Member Author

junnplus commented Jun 21, 2022

there are several ways how to set additionalGids field (cri, opts, and this one)

As far as I know, there are only these three.

whether it worth to consolidate those somehow.

WithAdditionalGIDs(opts) and WithAppendAdditionalGroups are used in different scenarios:

  • WithAdditionalGIDs: like docker/nerdctl run --user
  • WithAppendAdditionalGroups: like docker/nerdctl run --group-add

@dmcgowan dmcgowan merged commit 94fe150 into containerd:main Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dmcgowan dmcgowan dmcgowan approved these changes

@fuweid fuweid fuweid approved these changes

+1 more reviewer

@fahedouch fahedouch fahedouch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@junnplus @k8s-ci-robot @fahedouch @fuweid @mxpv @dmcgowan