systemd: Edit timers #22087

miguelbribeiro · 2025-06-09T09:23:00Z

Allow editing timers created with Cockpit on the web interface. The CreateTimerDialogBody was reused and the current settings are read and prefilled. Unfortunately, I couldn't come up with a good way of editing timers created externally: they support way too many settings to display in the UI, and you would need to fetch all of them from systemd and convert them back to the unit file format.

Related: #9467

pkg/systemd/timer-dialog-helpers.js

Venefilyn

Lots of good stuff here! Thanks for taking the time to go through this. I did some quick runthrough but need to test it a bit more.

@jelly do you have time to review this? Otherwise I can get to it at some point

pkg/systemd/timer-dialog.jsx

pkg/systemd/service-details.jsx

test/verify/check-system-services

Add a marker in the unit files of timers (and their services) created with Cockpit. Those timers can be edited by reading the current settings from the systemd's dbus API and reusing the CreateTimerDialog Related: cockpit-project#9467

miguelbribeiro · 2025-06-26T10:21:39Z

Sorry for the delay, I suddenly couldn't run the tests locally anymore and only had the time to fix it yesterday. I can squash the new commits if no more changes are needed.

jelly · 2025-06-27T09:21:51Z

pkg/systemd/service-details.jsx

    }

+    componentDidMount() {
+        cockpit.file(this.props.unit.FragmentPath).read()


This could avoid doing an expensive lookup for every time if we first checked if the FragmentPath includes /etc/systemd

jelly · 2025-06-27T09:23:34Z

pkg/systemd/service-details.jsx

+    componentDidMount() {
+        cockpit.file(this.props.unit.FragmentPath).read()
+                .then(content => {
+                    if (content.startsWith("# Managed by Cockpit.\n")) {


When navigating to an non-existing timer the page now raises an ooops. So we need to make sure FragmentPath isn't empty.

Uncaught TypeError: Cannot read properties of null (reading 'startsWith') at Promise2.<anonymous> (service-details.jsx:266:33)

jelly · 2025-06-27T09:35:55Z

pkg/systemd/service-details.jsx

+    editTimerAction() {
+        async function getCommand(bus, serviceUnitName) {
+            const serviceDbusPath = await bus.call(s_bus.O_MANAGER, s_bus.I_MANAGER, "LoadUnit", [serviceUnitName]);
+            const serviceProperties = await bus.call(serviceDbusPath[0], s_bus.I_PROPS, "GetAll", [s_bus.I_SERVICE]);


This needs some try/catch and useful error logging as far as I know LoadUnit can fail if the unit was for example edited by someone manually.

I read the dbus API documentation and there's nothing about LoadUnit failing in that case. I also tried editing an unit on my system and the call doesn't fail. Is there something I'm missing?

jelly · 2025-06-27T09:37:08Z

pkg/systemd/service-details.jsx

+            if (commands.length === 1) {
+                return join_command_arguments(commands[0][0], commands[0][1]);
+            } else {
+                return null;


How can this ever happen? No command? There should be an assert or at least a console.warn in case this happens.

jelly · 2025-06-27T09:46:36Z

pkg/systemd/timer-dialog-helpers.js

+    const result = [command];
+    for (const arg of args.slice(1)) {
+        // if argument has whitespace, enclose in quotes and escape quotes
+        if (/\s/.test(arg)) {


So this is for the case where ExecStart=/usr/bin/bash -c "echo hai" which is good common case to handle.

But: /usr/bin/bash -c 'echo "hai"' would also be valid and editing this would change the command.

This is maybe something where we want to run shlex.quote.

jelly · 2025-06-27T12:15:55Z

test/verify/check-system-services

+            time_ = b.val(f"[data-index='{index}'] .pf-v6-c-date-picker:nth-child(2) input")
+            self.assertIn((date, time_), (("2036-10-01", "01:22"), ("2036-08-01", "18:00")))
+
+        b.click("#timer-dialog-close-button")


Please wait till the dialog is closed, so add b.wait_not_present("#timer-dialog")

jelly · 2025-06-27T12:23:05Z

test/verify/check-system-services

+            self.assertIn((date, time_), (("2036-10-01", "01:22"), ("2036-08-01", "18:00")))
+
+        b.click("#timer-dialog-close-button")
+        b.go("/system/services#/?type=timer&owner=system")


This needs a wait condition to wait until the page is loaded, otherwise this will become a flaky test, so please add self.wait_page_load()

jelly · 2025-06-27T12:30:04Z

test/verify/check-system-services

+        self.allow_journal_messages("cockpit-session: admin: couldn't close session: System error: Unknown error -7")
+        self.reboot()
+
+        m.execute("timedatectl set-time '2036-01-01 15:30:00'")


All of above can be shared with testCreate in a new function

cockpituous · 2025-07-09T15:06:48Z

pkg/systemd/service-details.jsx

+            if (commands.length === 1) {
+                return join_command_arguments(commands[0][0], commands[0][1]);
+            } else {
+                return null;


This added line is not executed by any test. Details

cockpituous · 2025-07-09T15:06:48Z

pkg/systemd/timer-dialog-helpers.js

+    if (seconds % 604800 === 0) {
+        result.delayNumber = seconds / 604800;
+        result.delayUnit = "weeks";
+    } else if (seconds % 3600 === 0) {
+        result.delayNumber = seconds / 3600;
+        result.delayUnit = "hours";
+    } else if (seconds % 60 === 0) {
+        result.delayNumber = seconds / 60;
+        result.delayUnit = "minutes";


These 9 added lines are not executed by any test. Details

cockpituous · 2025-07-09T15:06:49Z

pkg/systemd/timer-dialog-helpers.js

+
+        return result;
+    } else {
+        return null;


This added line is not executed by any test. Details

cockpituous · 2025-07-09T15:06:49Z

pkg/systemd/timer-dialog.jsx

+                    {timer && !timer.delay && <FormAlert>
+                        <Alert variant="danger" title={_("Failed to get the starting conditions for the timer")} isInline />
+                    </FormAlert>}
+                    {timer && !timer.command && <FormAlert>
+                        <Alert variant="danger" title={_("Failed to get the timer command")} isInline />
+                    </FormAlert>}


These 6 added lines are not executed by any test. Details

cockpituous · 2025-07-09T15:06:49Z

pkg/systemd/timer-dialog.jsx

                                                                return arr;
                                                            })}
-                                                            appendTo={() => document.body} />
+                                                            appendTo={() => document.body}


This added line is not executed by any test. Details

Venefilyn · 2025-10-30T13:39:32Z

Heya! Just checking how it's going here @miguelbribeiro :)

miguelbribeiro · 2025-10-30T14:01:52Z

Heya! Just checking how it's going here @miguelbribeiro :)

Hey, I got busy with some other stuff back then and then kind of forgot about this PR. I'll try to finish it next week.

github-advanced-security bot found potential problems Jun 9, 2025

View reviewed changes

pkg/systemd/timer-dialog-helpers.js Fixed Show fixed Hide fixed

miguelbribeiro force-pushed the edit-timers branch 2 times, most recently from 86020e9 to 0a75f4a Compare June 18, 2025 10:44

Venefilyn requested changes Jun 18, 2025

View reviewed changes

miguelbribeiro added 5 commits June 25, 2025 13:32

systemd: Allow editing timers created by Cockpit

9fb1b7a

Add a marker in the unit files of timers (and their services) created with Cockpit. Those timers can be edited by reading the current settings from the systemd's dbus API and reusing the CreateTimerDialog Related: cockpit-project#9467

test: Add tests for editing timers

5014da0

Rename dialogs

e560a36

Move ifs out

2785361

Split timer creation into a separate function

5120279

miguelbribeiro force-pushed the edit-timers branch from 0a75f4a to 5120279 Compare June 25, 2025 12:44

miguelbribeiro requested a review from Venefilyn June 26, 2025 10:21

jelly reviewed Jun 27, 2025

View reviewed changes

cockpituous reviewed Jul 9, 2025

View reviewed changes

systemd: Edit timers #22087

Are you sure you want to change the base?

systemd: Edit timers #22087

Conversation

miguelbribeiro commented Jun 9, 2025

Uh oh!

Uh oh!

Venefilyn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miguelbribeiro commented Jun 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Venefilyn commented Oct 30, 2025

Uh oh!

miguelbribeiro commented Oct 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants