-
Notifications
You must be signed in to change notification settings - Fork 43
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
user: move userns package to separate module, and retract v0.2.0 #145
Conversation
4c83f2c
to
b088f05
Compare
@tianon @AkihiroSuda @corhere PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️ ❤️ ❤️
Makefile
Outdated
@@ -16,7 +16,7 @@ clean: | |||
test: test-local | |||
set -eu; \ | |||
for p in $(PACKAGES); do \ | |||
if $p = user && go version | grep -qv go1.18; then \ | |||
if $p = userns && go version | grep -qv go1.18; then \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, this syntax is funky -- if $p = userns
is going to run $p
as an executable with arguments = userns
and since $p
is a directory, will probably always be false and thus this block never runs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if $p = userns && go version | grep -qv go1.18; then \ | |
if [ "$p" = userns ] && go version | grep -qv go1.18; then \ |
(probably meant something like this here and in the other loops in this file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we only run the tests on go1.18? That seems strange? (I'm likely missing some context)
6867143
to
16e891a
Compare
2c44499
to
dda2929
Compare
Wah, sorry for the repeated push; I was fixing up the conditional skip; I gave up on trying to cramp it into the makefile as the combination of conditionals, subshells and windows became too fiddly; instead just made the list of modules to test conditional in the GitHub action @tianon PTAL if you're more happy with this approach 😅 |
@@ -14,6 +14,9 @@ jobs: | |||
go-version: ${{ matrix.go-version }} | |||
- name: Checkout code | |||
uses: actions/checkout@v4 | |||
- if: ${{ matrix.go-version == '1.18.x' }} | |||
run: | | |||
echo 'PACKAGES="mountinfo mount sequential signal symlink user"' >> $GITHUB_ENV |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same list as Makefile:1
, but minus userns
, right? That's somewhat clear while reviewing this diff while they're close together, but will no longer be clear the minute we merge this. 😅 How do we make sure we remember to update this list in the future? (and know how to update it correctly?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, no good solution for that, honestly. Unless we probably introduce more complexity.
Overall, the risk is low;
- if we don't update, the default is still all packages from the Makefile
- if a new module is added that doesn't support go1.18, that module will thus fail
- if we add a new module that should support go1.18, but we forgot to update; that module is skipped in the go1.18 tests (but still tested on current versions)
I'd consider only the last one slightly problematic, but perhaps a calculated risk, given that adding more modules should be a rare event 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
erm: middle bullet is obviously wrong; so only 1st and last apply 🤦♂️
dda2929
to
cedca31
Compare
commit 86870e7 integrated the userns package into the github.com/moby/sys/user module, which was included in the v0.2.0 version of the module. Upon further discussion with maintainers, this may not have been a good choice; the userns package is related to user-namespaces (uid/gid-mapping), and while there are some tangential relations with "user", we shouldn't conflate these concepts by putting both into the same module. Some downstream projects (containerd, moby, containerd/cgroups) already accepted patches to switch to the package that's part of the moby/sys/user module, but none of those patches made it into a release. This patch: - moves the userns package to a separate module - retracts the moby/sys/user v0.2.0 release - downgrades the minimum go version update for the moby/sys/user module to go1.17. Note that CI is no longer testing go1.17, but go1.18 as minimum. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
cedca31
to
370a9ed
Compare
# This corresponds with the list in Makefile:1, but omits the "userns" | ||
# module, which requires go1.21 as minimum. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment here to relate it to the variable in the Makefile
@@ -1,4 +1,4 @@ | |||
PACKAGES ?= mountinfo mount sequential signal symlink user | |||
PACKAGES ?= mountinfo mount sequential signal symlink user userns # IMPORTANT: when updating this list, also update the conditional one in .github/workflows/test.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And added a comment here as a reminder to check the one in the GitHub actions workflow when updating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🫶
Changes since Cory's review are pretty minimal overall, so I'm going to go ahead and merge, but of course we can always open a follow-up if doing so was premature. ❤️ |
Relates to:
internal
opencontainers/runc#3028Related downstream PRs;
commit 86870e7 integrated the userns package into the github.com/moby/sys/user module, which was included in the v0.2.0 version of the module.
Upon further discussion with maintainers, this may not have been a good choice; the userns package is related to user-namespaces (uid/gid-mapping), and while there are some tangential relations with "user", we shouldn't conflate these concepts by putting both into the same module.
Some downstream projects (containerd, moby, containerd/cgroups) already accepted patches to switch to the package that's part of the moby/sys/user module, but none of those patches made it into a release.
This patch: