Skip to content

Conversation

@p5
Copy link
Contributor

@p5 p5 commented May 28, 2025

- What I did

Reverted the systemd-sysusers change.

- Description for the changelog

@thaJeztah
Copy link
Member

Thanks! /cc @vvoland FYI

We'll probably be rebuilding the packages without the systemd-sysusers patch, and deploy new packages, keeping the systemd-sysusers change for a next release

This reverts commit 8c5e99f.

Signed-off-by: Robert Sturla <robertsturla@outlook.com>
@p5 p5 force-pushed the invoke-sysusers-postinstall branch from 5789968 to a76bedc Compare May 28, 2025 19:31
@p5
Copy link
Contributor Author

p5 commented May 28, 2025

@thaJeztah
I'm honestly thinking revert everything. I clearly don't understand this enough, and I wasn't sure my "fix" even fixed the RPMs.

I'm very sorry for the inconvenience here!

@p5 p5 changed the title fix(all): revert Debian sysusers, add RPM macro fix(all): revert sysusers May 28, 2025
@thaJeztah
Copy link
Member

Yes; that's what we want to do. looks like it's the last commit in the branch, so we can build using the commit before that (but we could do an explicit revert PR as well)

@p5
Copy link
Contributor Author

p5 commented May 28, 2025

Ok - please do whatever you can to fix the immediate issue of the broken release, and I'll figure out the proper fix to the issue.

I'm kinda strongly for reverting at least the Debian scripts since I'm completely in the dark on how it works, and it seems there's quite a bit of tech debt in this area.

Again, I'm very sorry for this!

@thaJeztah
Copy link
Member

No worries! We're also looking why our deploy pipeline didn't flag it; likely because it may have verified an upgrade on the same machine, so the user/group to have been there.

@p5
Copy link
Contributor Author

p5 commented May 28, 2025

I'm thinking the "proper" fix (i.e. to keep using sysusers across the board) would be to do something like:

diff --git a/deb/common/docker-ce.postinst b/deb/common/docker-ce.postinst
new file mode 100755
index 0000000..5bf9251
--- /dev/null
+++ b/deb/common/docker-ce.postinst
@@ -0,0 +1,20 @@
+#!/bin/sh
+set -e
+
+case "$1" in
+       configure)
+               if [ -z "$2" ]; then
+                       if ! getent group docker > /dev/null; then
+                               systemd-sysusers /usr/lib/sysusers.d/docker.conf
+                       fi
+               fi
+               ;;
+       abort-*)
+               # How'd we get here??
+               exit 1
+               ;;
+       *)
+               ;;
+esac
+
+#DEBHELPER#
diff --git a/rpm/SPECS/docker-ce.spec b/rpm/SPECS/docker-ce.spec
index 57d8a90..2a785d9 100644
--- a/rpm/SPECS/docker-ce.spec
+++ b/rpm/SPECS/docker-ce.spec
@@ -107,6 +107,9 @@ mkdir -p ${RPM_BUILD_ROOT}/etc/docker
 %{_mandir}/man*/*
 %dir /etc/docker
 
+%pre
+%systemd_sysusers_pre docker.conf
+
 %post
 %systemd_post docker.service

This uses a built-in macro for RPMs and adds back the postinstall command to the deb, which both invokes the sysusers file. Though in the other thread somebody said sysusers isn't used in Ubuntu and Debian, which I wasn't aware of.

How do you want to play this?
I'd be happer with a full revert, since it's turning out to be more problematic than you signed up for. We can attempt this later when we're not time pressured, where I'll create Ubuntu VMs to test the packages out with.

@p5 p5 marked this pull request as ready for review May 28, 2025 21:58
@vvoland vvoland merged commit c911c41 into docker:master May 30, 2025
13 checks passed
@vvoland
Copy link
Contributor

vvoland commented May 30, 2025

Reverted for now, let's open a new PR to discuss the new solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants