Use card view for remote management #403

tomasmatus · 2023-08-08T08:09:53Z

Follows up on #339

Old design:

New:

Pixel test changes: https://cockpit-logs.us-east-1.linodeobjects.com/pull-403-20231124-162305-47d51597-fedora-coreos/log.html

OSTree: Redesign with new features: status, clean up, reset, and pin

The OSTree page for software updates has been redesigned and includes several new features.

Update status is now listed in the "Status" card, and details about the current OSTree repository and branch are visible in the "OSTree source" card:

Unused deployments and package cache can be removed in using the "clean up" action.

A new "Reset" feature has been added, which can remove layered and overriden packages.

Deployments can be pinned to persist even when new deployments trigger an auto-cleanup and unpinned.

tomasmatus · 2023-08-08T08:27:10Z

A few things to discuss:

I changed branches select to a dropdown menu, some repositories have too many branches available and radio buttons weren't ideal option.

for example here's fedora repo:
For now I kept the option to add another gpg key in edit modal -- I am not sure how required this is and if it's something we should keep/drop
When remote refs not available error is shown current repository has no branches. In this case I followed old design but I was wondering if there's something smarter we should do instead of displaying error in a dropdown, such as hide the option to select branch but that might be too confusing.

This error likely isn't encountered often (if at all) when using official repositories.

Small note at the end - some features from the redesign (#176) aren't implemented yet. I need to figure out how to see if update includes bugfixes. Cleanup, reset, pin should come sometimes soon.

garrett

Thanks for implementing this!

It's not like the design, however.

The design is like this:

The branches are all exposed as radio buttons, as they should usually only be between 1 - 3 items. When something is 2 - 4 items, it should be a group of radios, not a dropdown. Dropdowns are better for > 5 items. When it's 1 item, then it should just be text, not a widget.

Additionally, the vertical space seems excessive?

Here's what one branch and the error state (which has a disabled button, as it cannot be used) would look like:

garrett · 2023-08-09T12:05:08Z

CoreOS only has 3 branches (next, stable, testing), not all the ones you listed (Silverblue, Sercea, etc.).

You're not going to switch architecture (going from x86_64 to s390, arm, etc. is a bad idea) and you shouldn't switch distributions (CoreOS to Silverblue or vice versa isn't a good idea in most cases).

garrett · 2023-08-09T14:46:46Z

Additionally, the vertical spacing isn't right in this screenshot:

(There's too much vertical space between the top two cards and the big bottom card.)

The vertical gap should be the same as the horizontal gap, such as in the mockup:

src/deploymentModals.jsx

garrett

Looks great! 👍

We should definitely filter the list and make it radios instead of a dropdown, but as we discussed in ~~IRC~~ Matrix, that can be a follow-up.

I also hit a bug where rebasing doesn't have a confirmation. It just rebases (without proper progress, and is uncancelable) and then instantly reboots when done. This needs fixing, but it's something unrelated. (It's why it took me longer to review this. Whoops! Sorry.)

I'd approve it (with the understanding there will be a PR to solve the above dropdown issue), but I have some questions about using custom CSS instead of PF helpers (and also possibly a PF Flex component instead of CSS).

src/ostree.scss

tomasmatus · 2023-10-06T09:57:01Z

I also hit a bug where rebasing doesn't have a confirmation. It just rebases (without proper progress, and is uncancelable) and then instantly reboots when done.

It's probably a good idea to add confirmation dialog for this. It may also be useful for rolling back and deleting previous deployments

tomasmatus · 2023-10-08T16:40:24Z

@garrett I added confirmation dialog for Updating, Rebasing and Roll backing:

I'm not sure if the confirm button should be red/yellow as this action is quite radical but at least there won't be any unexpected system reboots :)

garrett · 2023-10-11T16:22:23Z

So:

Having some kind of confirmation modal is a good idea.
The modal should provide specific information, such as the version being rebased to (and possibly from, although I guess that doesn't matter as much), and possibly how long ago it was released.
It currently doesn't use our standard reboot modal (with a time delay and messaging). Other parts of Cockpit, including the reboot button (but not only). Use this. I think PackageKit does too and would be a good idea to refer to for the OSTree page?

So, perhaps rework this PR to be like the first mockup here?

The second mockup is what it might look like if we had our scheduling from the standard reboot modal.

garrett · 2023-10-11T16:25:00Z

Perhaps it should be a warning. And I think PF would suggest an icon there too.

jelly · 2023-11-01T12:53:53Z

Few things I noticed which might be unrelated:

No loading for rebase modal

So you go from an error message to we have a list of things to pick from:

Cleanup modal

I would be surprised if this goes past @garrett, the action button name feels wrong as one of the options is "Temporary files". Maybe this should be Clean up or Delete? Looking at design guidelines I'd go for Cleanup, that seems to fit the pattern.

Warning icon missing?

Same comment as above basically.

Deleting your only deployment is an option in the UI but won't work for obvious reasons

See the error in console: error: Cannot undeploy currently booted deployment 0

Pinning unstaged deployment

See the error in console: error: Cannot pin staged deployment

To reproduce rpm-ostree install git or maybe something smaller :)

jelly

With limited OSTree experience, overall it looks quite good. There are some things I noticed which we might want to improve.

src/deploymentModals.jsx

garrett · 2023-11-14T10:09:12Z

Overall, great catches and suggestions! Thanks!

Details below:

Cleanup modal

It's never "cleanup" for the verb. It would be "clean up".

Cleanup is a noun only (or sometimes an adjective in baseball)... and even then, it's usually slang or informal:
https://www.wordnik.com/words/cleanup

Additionally, it feels like this modal grew in scope and the language hasn't caught up. It's fine, and I'm glad it covers all these items.

We might want to drop the description and make the heading "Remove temporary files" and make the action "Clean up".

Additionally, if someone went to this dialog, they probably want to clean things. So I'd suggest having a default of temporary files and RPM repo metadata preselected. Pending deployment and rollback deployment can change things, but the other two items would have no effect on the system (other than re-dowloading some files).

Warning icon missing?

Yep. It should have the warning icon in the title.

Deleting your only deployment is an option in the UI but won't work for obvious reasons

It should not be an option in the UI.

Pinning unstaged deployment

Right, you shouldn't be able to pin an unstaged deployment. But you should be able to pin the current one. (On the command line, if you have a staged deployment, then it's ostree admin pin 1 instead of ostree admin pin 0, as 0 is the new deployment.)

tomasmatus · 2023-11-15T12:12:00Z

Everything mentioned above should be addressed now.

Reset modal now has warning icon

Rebase modal has a loading indicator

Changed text in clean up modal (first two options are selected by default)

jelly · 2023-11-21T10:35:33Z

Probably unrelated to these changes if you run rpm-ostree install and click on refresh the page breaks.

test/check-ostree

src/ostree.jsx

src/deploymentModals.jsx

src/ostree.jsx

src/repositoryModals.jsx

jelly · 2023-11-23T15:29:06Z

This ID no longer exists so needs to be updated:

Traceback (most recent call last):
  File "/work/bots/make-checkout-workdir/test/check-ostree", line 215, in testOstree
    b.assert_pixels("#repo-remote-toolbar", "remote-toolbar")
  File "/work/bots/make-checkout-workdir/test/common/testlib.py", line 1197, in assert_pixels
    self.assert_pixels_in_current_layout(selector, key, ignore=ignore,
  File "/work/bots/make-checkout-workdir/test/common/testlib.py", line 1022, in assert_pixels_in_current_layout
    self.call_js_func('ph_scrollIntoViewIfNeeded', scroll_into_view or selector)
  File "/work/bots/make-checkout-workdir/test/common/testlib.py", line 344, in call_js_func
    return self.eval_js("%s(%s)" % (func, ','.join(map(jsquote, args))))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/work/bots/make-checkout-workdir/test/common/testlib.py", line 325, in eval_js
    self.raise_cdp_exception("eval_js", code, result["exceptionDetails"])
  File "/work/bots/make-checkout-workdir/test/common/testlib.py", line 305, in raise_cdp_exception
    raise Error("%s(%s): %s" % (func, arg, msg))
testlib.Error: eval_js(ph_scrollIntoViewIfNeeded("#repo-remote-toolbar")): Uncaught

jelly · 2023-11-24T09:44:54Z

src/deploymentModals.jsx

+        if (deleteTemporaryFiles) {
+            cleanupFlags.push("base");


Take away from coverage, this and below could be tested if possible.

jelly · 2023-11-24T09:46:21Z

src/deploymentModals.jsx

+            // remove all overlayed packages
+            resetFlags["no-layering"] = { t: "b", v: true };
+        }
+        if (removeOverrides) {


This is also untested, could be interesting if trivial.

jelly · 2023-11-24T12:34:56Z

P.S. this also needs a release-note but that can happen Monday/Tuesday

jelly · 2023-11-28T11:20:35Z

Pushed the pixel tests as they look ok to me.

jelly

Thanks!

cockpituous · 2023-11-29T09:40:25Z

src/deploymentModals.jsx

+                .catch(ex => {
+                    console.warn(ex);
+                    setError(ex.message);
+                    setButtonLoading(false);


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

cockpituous · 2023-11-29T09:40:25Z

src/deploymentModals.jsx

+    const actions = [
+        <Button key="cleanup-accept"
+            variant="primary"
+            isDisabled={buttonLoading || (!deleteTemporaryFiles && !deleteRPMmetadata && !deletePendingDeployments && !deleteRollbackDeployments)}


This added line is not executed by any test. Details

cockpituous · 2023-11-29T09:40:25Z

src/deploymentModals.jsx

+                <Alert variant="danger"
+                    isInline
+                    title={error}


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

cockpituous · 2023-11-29T09:40:25Z

src/deploymentModals.jsx

+                <Checkbox label={_("Temporary files")}
+                    key="temporary-files"
+                    id="temporary-files-checkbox"
+                    onChange={(_, isChecked) => setDeleteTemporaryFiles(isChecked)}


This added line is not executed by any test. Details

cockpituous · 2023-11-29T09:40:25Z

src/deploymentModals.jsx

+                <Checkbox label={_("RPM repo metadata")}
+                    key="rpm-repo-metadata"
+                    id="rpm-repo-metadata-checkbox"
+                    onChange={(_, isChecked) => setDeleteRPMmetadata(isChecked)}


This added line is not executed by any test. Details

cockpituous · 2023-11-29T09:40:26Z

src/repositoryModals.jsx

+                        onChange={(_ev, url) => setNewRepoURL(url)}
+                    />
+                    <FormHelper fieldId="new-remote-url"
+                        helperTextInvalid={(hasValidation && !newRepoURL.trim().length) && _("Please provide a valid URL")}


This added line is not executed by any test. Details

cockpituous · 2023-11-29T09:40:26Z

src/repositoryModals.jsx

+                >
+                    <TextInput id="edit-remote-url"
+                        value={newURL}
+                        onChange={(_ev, url) => setNewURL(url)}


This added line is not executed by any test. Details

cockpituous · 2023-11-29T09:40:26Z

src/repositoryModals.jsx

+                    if (newBranches.includes(origin.branch)) {
+                        setSelectedBranch(origin.branch);


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

cockpituous · 2023-11-29T09:40:26Z

src/repositoryModals.jsx

+            </Select>
+        )
+        : (
+            <Text>{availableRemotes[0]}</Text>


This added line is not executed by any test. Details

cockpituous · 2023-11-29T09:40:27Z

src/repositoryModals.jsx

+            onClose={Dialogs.close}
+            actions={actions}
+        >
+            {error && <Alert variant="danger" isInline title={error} />}


This added line is not executed by any test. Details

stale

tomasmatus force-pushed the page-redesign branch 2 times, most recently from b4e0fa3 to 27537be Compare August 9, 2023 11:23

garrett requested changes Aug 9, 2023

View reviewed changes

tomasmatus force-pushed the page-redesign branch 6 times, most recently from 340fea4 to 5426a54 Compare August 22, 2023 14:15

tomasmatus force-pushed the page-redesign branch from 5426a54 to 7d1520d Compare August 24, 2023 11:10

github-advanced-security bot found potential problems Aug 24, 2023

View reviewed changes

src/deploymentModals.jsx Fixed Show fixed Hide fixed

tomasmatus force-pushed the page-redesign branch 3 times, most recently from 5a955f6 to e09dae7 Compare September 4, 2023 12:26

tomasmatus force-pushed the page-redesign branch from e09dae7 to 071e781 Compare September 14, 2023 13:57

tomasmatus force-pushed the page-redesign branch 2 times, most recently from a2d41a0 to 7c487bc Compare October 5, 2023 12:47

garrett previously requested changes Oct 5, 2023

View reviewed changes

src/ostree.scss Outdated Show resolved Hide resolved

src/ostree.scss Outdated Show resolved Hide resolved

tomasmatus force-pushed the page-redesign branch from 7c487bc to a0d6797 Compare October 6, 2023 09:53

tomasmatus force-pushed the page-redesign branch from a0d6797 to ea850dd Compare October 8, 2023 16:32

tomasmatus requested a review from jelly October 11, 2023 14:06

tomasmatus force-pushed the page-redesign branch from ea850dd to 04fcbf7 Compare October 14, 2023 09:40

jelly requested changes Nov 1, 2023

View reviewed changes

src/deploymentModals.jsx Show resolved Hide resolved

src/deploymentModals.jsx Outdated Show resolved Hide resolved

src/deploymentModals.jsx Outdated Show resolved Hide resolved

tomasmatus force-pushed the page-redesign branch from 04fcbf7 to 63a7351 Compare November 8, 2023 16:22

tomasmatus force-pushed the page-redesign branch from 63a7351 to b03c971 Compare November 15, 2023 12:07

jelly reviewed Nov 21, 2023

View reviewed changes

tomasmatus force-pushed the page-redesign branch from b03c971 to 28d21e5 Compare November 22, 2023 10:14

jelly reviewed Nov 24, 2023

View reviewed changes

jelly added the release-note label Nov 24, 2023

tomasmatus force-pushed the page-redesign branch 2 times, most recently from 9bde110 to 47d5159 Compare November 24, 2023 16:22

jelly added the release-blocker label Nov 28, 2023

jelly force-pushed the page-redesign branch 2 times, most recently from 30a3544 to 91f0c34 Compare November 28, 2023 14:47

Use card view for remote management

cdaefe6

tomasmatus force-pushed the page-redesign branch from 91f0c34 to cdaefe6 Compare November 29, 2023 09:15

jelly approved these changes Nov 29, 2023

View reviewed changes

cockpituous reviewed Nov 29, 2023

View reviewed changes

martinpitt merged commit 7c6271c into cockpit-project:main Nov 29, 2023
6 checks passed

martinpitt mentioned this pull request Nov 29, 2023

Fix titles #464

Merged

jelly removed the release-note label Dec 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use card view for remote management #403

Use card view for remote management #403

tomasmatus commented Aug 8, 2023 •

edited

Loading

tomasmatus commented Aug 8, 2023 •

edited

Loading

garrett left a comment •

edited

Loading

garrett commented Aug 9, 2023 •

edited

Loading

garrett commented Aug 9, 2023

garrett left a comment •

edited

Loading

tomasmatus commented Oct 6, 2023

tomasmatus commented Oct 8, 2023

garrett commented Oct 11, 2023

garrett commented Oct 11, 2023

jelly commented Nov 1, 2023 •

edited

Loading

jelly left a comment

garrett commented Nov 14, 2023 •

edited

Loading

tomasmatus commented Nov 15, 2023

jelly commented Nov 21, 2023

jelly commented Nov 23, 2023

jelly Nov 24, 2023

jelly Nov 24, 2023

jelly commented Nov 24, 2023

jelly commented Nov 28, 2023

jelly left a comment

cockpituous Nov 29, 2023

cockpituous Nov 29, 2023

cockpituous Nov 29, 2023

cockpituous Nov 29, 2023

cockpituous Nov 29, 2023

cockpituous Nov 29, 2023

cockpituous Nov 29, 2023

cockpituous Nov 29, 2023

cockpituous Nov 29, 2023

cockpituous Nov 29, 2023

		if (newBranches.includes(origin.branch)) {
		setSelectedBranch(origin.branch);

Use card view for remote management #403

Use card view for remote management #403

Conversation

tomasmatus commented Aug 8, 2023 • edited Loading

OSTree: Redesign with new features: status, clean up, reset, and pin

tomasmatus commented Aug 8, 2023 • edited Loading

garrett left a comment • edited Loading

Choose a reason for hiding this comment

garrett commented Aug 9, 2023 • edited Loading

garrett commented Aug 9, 2023

garrett left a comment • edited Loading

Choose a reason for hiding this comment

tomasmatus commented Oct 6, 2023

tomasmatus commented Oct 8, 2023

garrett commented Oct 11, 2023

garrett commented Oct 11, 2023

jelly commented Nov 1, 2023 • edited Loading

No loading for rebase modal

Cleanup modal

Warning icon missing?

Deleting your only deployment is an option in the UI but won't work for obvious reasons

Pinning unstaged deployment

jelly left a comment

Choose a reason for hiding this comment

garrett commented Nov 14, 2023 • edited Loading

tomasmatus commented Nov 15, 2023

jelly commented Nov 21, 2023

jelly commented Nov 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelly commented Nov 24, 2023

jelly commented Nov 28, 2023

jelly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomasmatus commented Aug 8, 2023 •

edited

Loading

tomasmatus commented Aug 8, 2023 •

edited

Loading

garrett left a comment •

edited

Loading

garrett commented Aug 9, 2023 •

edited

Loading

garrett left a comment •

edited

Loading

jelly commented Nov 1, 2023 •

edited

Loading

garrett commented Nov 14, 2023 •

edited

Loading