-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 #21335
base: main
Are you sure you want to change the base?
Changes from all commits
e0f2280
ab4302e
4cc003e
eecfb6a
0a8d287
ec698d4
0924383
5c1232f
d1dcb7f
575f84b
d1089ae
8523738
bf240b5
8b21fe4
4d8c106
9be58b2
2517341
796b087
36eee2b
d7ec9fd
8bd5ad4
6e78bd5
e71a3e2
ec0b8ef
8797635
3a04ac5
b1f4327
d4fcabb
adc139a
e531951
89abe03
37ed3bd
d43386d
fd91467
bc781e4
78beade
a55f6f4
1cee618
e8408c3
7c04921
e57f1fb
dba53de
6703137
d722fbc
6b79abd
fa069fa
916efb9
cd30555
c878e1e
4ed8264
80e8852
6b8f564
4c242ee
832c835
20bb19e
17c5595
58e6d98
64ec395
05fc7c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,6 +59,7 @@ Makefile.in | |
/tags | ||
/test-* | ||
/test_rsa_key | ||
/Test[A-Z]* | ||
/tmp-dist | ||
/tmp/ | ||
/tsconfig.tsbuildinfo | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -556,10 +556,24 @@ class AddEditServicesModal extends React.Component { | |
componentDidMount() { | ||
firewall.getAvailableServices() | ||
.then(services => this.setState({ services })); | ||
cockpit.file('/etc/services').read() | ||
.then(content => this.setState({ | ||
avail_services: this.parseServices(content) | ||
})); | ||
async function fetchServices(component) { | ||
try { | ||
await cockpit.spawn(["test", "-e", "/usr/etc/services"], { err: "ignore" }); | ||
cockpit.file('/usr/etc/services').read() | ||
.then(content => component.setState({ | ||
avail_services: component.parseServices(content) | ||
})); | ||
} catch (err1) { | ||
Comment on lines
+561
to
+566
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
cockpit.file('/etc/services').read() | ||
.then(content => component.setState({ | ||
avail_services: component.parseServices(content) | ||
})); | ||
} | ||
} | ||
|
||
if (this.state.avail_services === null) { | ||
fetchServices(this); | ||
} | ||
} | ||
|
||
onFilterChanged(value) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,17 +50,29 @@ export class MultipathAlert extends React.Component { | |
const multipathd_running = !this.multipathd_service.state || this.multipathd_service.state === "running"; | ||
const multipath_broken = client.broken_multipath_present === true; | ||
|
||
function activate(event) { | ||
async function activate(event) { | ||
if (!event || event.button !== 0) | ||
return; | ||
cockpit.spawn(["mpathconf", "--enable", "--with_multipathd", "y"], | ||
{ superuser: "try" }) | ||
.catch(function (error) { | ||
dialog_open({ | ||
Title: _("Error"), | ||
Body: error.toString() | ||
try { | ||
await cockpit.spawn(["type", "mpathconf"], { err: "ignore" }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "type" is a shell builtin, you can't (reliably) call this as command. You need |
||
cockpit.spawn(["mpathconf", "--enable", "--with_multipathd", "y"], | ||
{ superuser: "try" }) | ||
.catch(function (error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't mix Promises and async . |
||
dialog_open({ | ||
Title: _("Error"), | ||
Body: error.toString() | ||
}); | ||
}); | ||
}); | ||
} catch (err1) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to check the specific error (should be |
||
cockpit.spawn(["systemctl", "enable", "--now", "multipathd.service"], | ||
{ superuser: "try" }) | ||
.catch(function (error) { | ||
dialog_open({ | ||
Title: _("Error"), | ||
Body: error.toString() | ||
}); | ||
}); | ||
} | ||
} | ||
|
||
if (multipath_broken && !multipathd_running) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,7 +91,14 @@ function AccountsPage() { | |
const handleShadow = cockpit.file("/etc/shadow", { superuser: "try" }); | ||
handleShadow.watch(() => debouncedGetLoginDetails(), { read: false }); | ||
|
||
const handleLogindef = cockpit.file("/etc/login.defs"); | ||
let handleLogindef; | ||
try { | ||
await cockpit.spawn(["test", "-e", "/etc/login.defs"], { err: "ignore" }); | ||
handleLogindef = cockpit.file("/etc/login.defs"); | ||
} catch (err1) { | ||
handleLogindef = cockpit.file("/usr/etc/login.defs"); | ||
} | ||
Comment on lines
+94
to
+100
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is being watched, which doesn't make sense for /usr/etc/ -- that isn't supposed to change. You always need to watch /etc/login.defs even if it doesn't initially exist (that is fine). You can do the initial |
||
|
||
handleLogindef.watch((logindef) => { | ||
if (logindef === null) | ||
return; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,7 +76,7 @@ regcompx (regex_t *preg, const char *regex, int cflags) | |
/* For optimization, this modifies string; the returned array has pointers into string | ||
* The array itself gets allocated and must be freed after use. */ | ||
static const char ** | ||
strsplit (char *string, char delimiter) | ||
strsplit (char *string, char delimiter, unsigned *len_out) | ||
{ | ||
const char ** parts = reallocarrayx (NULL, 2, sizeof (char*)); | ||
char *cur = string; | ||
|
@@ -104,6 +104,10 @@ strsplit (char *string, char delimiter) | |
} | ||
|
||
parts[len] = NULL; | ||
|
||
if (len_out) | ||
*len_out = len; | ||
|
||
return parts; | ||
} | ||
|
||
|
@@ -299,9 +303,21 @@ cockpit_conf_get_dirs (void) | |
env = getenv ("XDG_CONFIG_DIRS"); | ||
if (env && env[0]) | ||
{ | ||
unsigned len_out; | ||
|
||
const char ** xdg_config_dirs = NULL; | ||
unsigned len = sizeof (cockpit_config_dirs) / sizeof (char*); | ||
Comment on lines
+306
to
+309
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please describe in more detail what "cockpitconf: Always check against system config directories" does and why it's needed? I haven't reviewed this in detail, but as this is much less straightforward, please split it into its own PR. |
||
system_config_dirs = reallocarrayx (NULL, len, sizeof (char*)); | ||
memcpy (system_config_dirs, cockpit_config_dirs, sizeof (cockpit_config_dirs)); | ||
|
||
/* strsplit() modifies the string inline, so copy and keep a ref */ | ||
env = strdup (env); | ||
system_config_dirs = strsplit (env, ':'); | ||
xdg_config_dirs = strsplit (env, ':', &len_out); | ||
|
||
system_config_dirs = reallocarrayx (system_config_dirs, len + len_out + 1, sizeof (char*)); | ||
memcpy (system_config_dirs + (len-1), xdg_config_dirs, len_out * sizeof (char*)); | ||
system_config_dirs[len - 1 + len_out] = NULL; | ||
free (xdg_config_dirs); | ||
} | ||
} | ||
|
||
|
@@ -348,7 +364,7 @@ cockpit_conf_strv (const char *section, | |
entry->strv_value = strdupx (entry->value); | ||
for (char *c = entry->strv_value + strlen (entry->strv_value) - 1; c >= entry->strv_value && isspace (*c); --c) | ||
*c = '\0'; | ||
entry->strv_cache = strsplit (entry->strv_value, delimiter); | ||
entry->strv_cache = strsplit (entry->strv_value, delimiter, NULL); | ||
entry->strv_delimiter = delimiter; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,8 +44,11 @@ def add_veth(self, name, dhcp_cidr=None, dhcp_range=None): | |
if dhcp_cidr: | ||
# up the remote end, give it an IP, and start DHCP server | ||
self.machine.execute(f"ip a add {dhcp_cidr} dev v_{name}; ip link set v_{name} up") | ||
server = self.machine.spawn("dnsmasq --keep-in-foreground --log-queries --log-facility=- " | ||
f"--conf-file=/dev/null --dhcp-leasefile=/tmp/leases.{name} --no-resolv " | ||
|
||
# TODO: Consider running all env's through /run/dnsmasq | ||
lease_path = "/run/dnsmasq" if "suse" in self.machine.image else "/tmp" | ||
Comment on lines
+48
to
+49
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's fine to apply that to all OSes. |
||
server = self.machine.spawn(f"mkdir -p {lease_path}; dnsmasq --keep-in-foreground --log-queries --log-facility=- " | ||
f"--conf-file=/dev/null --dhcp-leasefile={lease_path}/leases.{name} --no-resolv " | ||
f"--bind-interfaces --except-interface=lo --interface=v_{name} --dhcp-range={dhcp_range[0]},{dhcp_range[1]},4h", | ||
f"dhcp-{name}.log") | ||
self.addCleanup(self.machine.execute, "kill %i" % server) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,6 +46,10 @@ def setUp(self): | |
self.backend = "dnf5" | ||
self.primary_arch = "noarch" | ||
self.secondary_arch = "x86_64" | ||
elif "suse" in self.machine.image: | ||
self.backend = "zypper" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. packagelib.py doesn't actually implement a "zypper" backend. Maybe a future commit will introduce this, but please squash these together then. As a standalone commit this is broken. |
||
self.primary_arch = "noarch" | ||
self.secondary_arch = "x86_64" | ||
elif self.machine.image == "arch": | ||
self.backend = "alpm" | ||
self.primary_arch = "any" | ||
|
@@ -157,7 +161,7 @@ def createPackage(self, name, version, release, install=False, | |
else: | ||
self.createRpm(name, version, release, depends, postinst, install, content, arch, provides) | ||
if updateinfo: | ||
self.updateInfo[(name, version, release)] = updateinfo | ||
self.updateInfo[name, version, release] = updateinfo | ||
|
||
def createDeb(self, name, version, depends, postinst, install, content, arch, provides): | ||
"""Create a dummy deb in repo_dir on self.machine | ||
|
@@ -262,6 +266,13 @@ def createRpm(self, name, version, release, requires, post, install, content, ar | |
cp ~/rpmbuild/RPMS/{4}/*.rpm {0} | ||
rm -rf ~/rpmbuild | ||
""" | ||
if self.backend == "zypper": | ||
cmd = """ | ||
Comment on lines
+269
to
+270
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AH, here it is. About fifty commits ago I complained about an incomplete packagelib.py commit -- that "test: Add in handling for package cases using zypper backend" is the one with which it should be squashed 😁 |
||
rpmbuild --quiet -bb /tmp/spec | ||
mkdir -p {0} | ||
cp /usr/src/packages/RPMS/{4}/*.rpm {0} | ||
""" | ||
|
||
if install: | ||
cmd += "rpm -i {0}/{1}-{2}-{3}.*.rpm" | ||
self.machine.execute(cmd.format(self.repo_dir, name, version, release, arch)) | ||
|
@@ -435,6 +446,10 @@ def enableRepo(self): | |
self.machine.write("/etc/pacman.conf", config) | ||
self.machine.execute("pacman -Sy") | ||
|
||
elif self.backend == "zypper": | ||
# Need to work out how zypper handles repo changelogs so we can handle that here | ||
self.machine.execute(f"zypper ar --no-gpgcheck --refresh {self.repo_dir} local") | ||
|
||
else: | ||
# HACK - https://bugzilla.redhat.com/show_bug.cgi?id=2306114 | ||
# We need to explicitly create /var/cache/libdnf5 to make "dnf clean" happy. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1094,7 +1094,7 @@ def become_superuser( | |
|
||
# In (open)SUSE images, superuser access always requires the root password | ||
if user is None: | ||
user = "root" if "suse" in self.machine.image else "admin" | ||
user = get_superuser(self.machine.image) | ||
|
||
if passwordless: | ||
self.wait_in_text("div[role=dialog]", "Administrative access") | ||
|
@@ -1776,7 +1776,7 @@ def setUp(self, restrict: bool = True) -> None: | |
self.sshd_socket = 'ssh.socket' | ||
else: | ||
self.sshd_service = 'sshd.service' | ||
if image == 'arch': | ||
if image == 'arch' or "suse" in image: | ||
self.sshd_socket = None | ||
else: | ||
self.sshd_socket = 'sshd.socket' | ||
|
@@ -1785,7 +1785,7 @@ def setUp(self, restrict: bool = True) -> None: | |
# only enabled by default on released OSes; see pkg/shell/manifest.json | ||
self.multihost_enabled = image.startswith(("rhel-9", "centos-9")) or image in [ | ||
"ubuntu-2204", "ubuntu-2404", "debian-stable", | ||
"fedora-39", "fedora-40"] | ||
"fedora-39", "fedora-40", "opensuse-tumbleweed"] | ||
# Transitional code while we move ubuntu-stable from 24.04 to 24.10 | ||
if image == "ubuntu-stable" and m.execute(". /etc/os-release; echo $VERSION_ID").strip() == "24.04": | ||
self.multihost_enabled = True | ||
|
@@ -2052,6 +2052,12 @@ def login_and_go( | |
|
||
# timedatex.service shuts down after timeout, runs into race condition with property watching | ||
".*org.freedesktop.timedate1: couldn't get all properties.*Error:org.freedesktop.DBus.Error.NoReply.*", | ||
|
||
# https://github.com/cockpit-project/cockpit/issues/19235 | ||
"invalid non-UTF8 @data passed as text to web_socket_connection_send.*", | ||
Comment on lines
+2056
to
+2057
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nack - this is an extremely serious bug, and we can't just hide it. Also, that bug got fixed in June? |
||
|
||
# Noise from btmp file missing systems where it doesn't exists | ||
r"cockpit-session: open\(\/var\/log\/btmp\) failed: No such file or directory", | ||
Comment on lines
+2059
to
+2060
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather fix that in src/session/session-utils.c |
||
] | ||
|
||
default_allowed_messages += os.environ.get("TEST_ALLOW_JOURNAL_MESSAGES", "").split(",") | ||
|
@@ -2467,6 +2473,15 @@ def get_decorator(method: object, _class: object, name: str, default: Any = None | |
return getattr(method, attr, getattr(_class, attr, default)) | ||
|
||
|
||
def get_superuser(image: str) -> str: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do I understand this right? The user is still called "admin", but sudo asks for the root password instead of admins'? This wouldn't apply to polkit, so perhaps |
||
# In (open)SUSE images, superuser access always requires the root password | ||
return "root" if "suse" in image else "admin" | ||
|
||
|
||
def get_sshd_config_path(image: str) -> str: | ||
# In (open)SUSE images, superuser access always requires the root password | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That comment is wrong. |
||
return "/usr/etc/ssh/sshd_config" if "suse" in image else "/etc/ssh/sshd_config" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is wrong. Like in Fedora OSTree, /usr/etc are the read-only original config files shipped by the package. The tests will run I suggest to split this out into its own PR -- please do a draft with ssh_config.d/, and I'm happy to fix it up for old distros. |
||
|
||
########################### | ||
# Test decorators | ||
# | ||
|
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.
Please change "system: Add support for suse platforms" to "lib: Add Suse support to kernelopt.sh"