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

Add testing support for OpenSUSE Tumbleweed #7143

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

SludgeGirl
Copy link

This pairs with cockpit-project/cockpit#21335

This is aimed to add most of the support for testing Tumbleweed, let me know if you need any extra changes made to it

Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks! Initial review -- none of that is a veto of course, in the end this is "your" image, but let's at least discuss this a bit.

Can you please squash this? Like, put all the new dependencies into one commit, and squash the redis/valkey commits, and such.

Comment on lines +142 to +143
# Setup user friendly names for multipath
printf 'defaults {\nuser_friendly_names yes\n}\n' > /etc/multipath.conf
Copy link
Member

Choose a reason for hiding this comment

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

I leave this up to you, but this is a smell. It's better to test the OS at it is for real users, and deal with the fallout in our code and tests. If this is a workaround for a bug in TW, it's useful to add a reference to it in a comment and prefix it with # HACK:. We use that a lot, then it remains greppable.

@@ -98,6 +98,8 @@ if [ "${IMAGE%-i386}" != "$IMAGE" ]; then
fi

zypper dup -y

# Install default kernel for testing PLEASE REMOVE
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, what's the condition to remove this? The comment below already gives the rationale.

Comment on lines +118 to +121
# cloud-init setups the wheel group incorrectly so we need to recreate it
groupdel wheel
zypper install -y system-group-wheel
sudo usermod -a -G wheel admin
Copy link
Member

Choose a reason for hiding this comment

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

This also feels like working around the OS, and should get a bug reference. In what way is "wheel" wrong? I.e. in a few months time how can developers decide if this can be dropped?

@@ -37,7 +37,7 @@ echo "$spec" | rpmspec -D "$OS_VER_NO_VARIANT" -D 'version 0' -D 'enable_old_bri
# - nodejs for starter-kit and other projects which rebuild webpack during RPM build
case "$OS_VER" in
*suse*)
EXTRA_DEPS="appstream-glib rpmlint gettext-runtime desktop-file-utils nodejs-default"
EXTRA_DEPS="appstream-glib rpmlint gettext-runtime desktop-file-utils nodejs-default system-group-wheel"
Copy link
Member

Choose a reason for hiding this comment

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

This is for configure.ac to auto-detect --with-admin-group= presumably?

Comment on lines 148 to 156
# Setup redis
sudo cp /etc/redis/default.conf.example /etc/redis/redis.conf
sudo chown redis:redis /etc/redis/redis.conf

mkdir -p /etc/systemd/system/redis.target.d
cat > /etc/systemd/system/redis.target.d/70-redis-targets.conf <<EOF
[Unit]
[email protected]
EOF
Copy link
Member

Choose a reason for hiding this comment

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

Is that documented in TW somehow? That's all stuff that should either happen by default (to get an useful experience out of the box) or in Cockpit's metrics page. It doesn't help users if tests go green with "it works against a hacked up image", but not against a default install.

[email protected]
EOF

systemctl enable --now redis.target
Copy link
Member

Choose a reason for hiding this comment

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

I'd advise against that. We don't enable it in Fedora/RHEL images by default, and even disable it in Debian. Most tests don't need it, and Cockpit's metrics page starts it if/when necessary.

Comment on lines +115 to +116
# use unpredictable network interface names
ln -s /dev/null /etc/systemd/network/99-default.link
Copy link
Member

Choose a reason for hiding this comment

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

This is another case of "please test the real-world default OS, not some hack". We fixed our projects to get along without hardcoded ethernet names a while ago, this really shouldn't be necessary.

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.

3 participants