apps: Warn if appstream data package is missing #19281

leomoty · 2023-09-04T14:20:02Z

Goes on top of apps: Fix reading AppStream metadata config #19324
Goes on top of apps: Fix crash with nonexisting /etc/os-release #19416

Apps: Detect missing AppStream metadata

If the AppStream metadata is not installed, the "Apps" page cannot show available Cockpit extensions. The page detects this now, and offers the user to install the metadata.

Thanks to leomoty for designing this improvement!

martinpitt

Thanks! This also needs an integration test. Do you want to work on that, or need some help?

pkg/apps/application-list.jsx

leomoty · 2023-09-04T15:51:32Z

Do you want to work on that, or need some help?

I would like to give it a try, I was writing this before:

    def testNoAppStream(self):
        b = self.browser
        m = self.machine

        m.execute("dnf remove -y appstream")

        self.login_and_go("/apps")

        b.click("#refresh")

        with b.wait_timeout(30):
          b.wait_in_text(".pf-v5-c-modal-box:contains('Install software')", "appstream will be installed")

leomoty · 2023-09-04T15:56:43Z

Added an attempt assert for the Alert also, but VM says no:

it is not installable, so it doesn't hit my alert, should we handle unavailable_names as well, and if so, what would it do?

leomoty · 2023-09-04T16:10:21Z

@martinpitt the only caveat is that I have no idea how to get appstream installed to test otherwise, but seems like everything else was already mocking anyways.

The refresh will remain blocked this way, btw.

pkg/apps/application-list.jsx

leomoty · 2023-09-04T16:20:12Z

Btw it breaks testBasic and TestWithUrlRoot due to its reliance on #refresh button.

Edit: solved

leomoty · 2023-09-04T22:19:48Z

Had some troubles with tox but fixed now, but overall I believe the only thing missing is my own question

martinpitt

Sorry, I misunderstood the purpose of this yesterday. What this PR should do is to show the alert if the metapackages are absent. It should not modify the actual install procedure, as that is already happening. See the detailed comments below.

Added an attempt assert for the Alert also, but VM says no:

That's because you added a new test case which doesn't call self.createAppStreamRepoPackage() to mock the metapackages. But this should go away entirely.

Thanks!

test/verify/check-apps

leomoty · 2023-09-06T00:35:38Z

This should be rebased after #19283, to test I just changed the manifest.json locally to make rocky happy. I wasn't sure if there is any chance for it not to be present after a refresh, so just in case I made sure to retry it.

Let me know if you want further changes.

leomoty · 2023-09-07T11:24:57Z

@martinpitt I've rebased on top of main, testOsMap fails locally for me, but I am not entirely sure why, i'll keep digging, but feel free to fix if obvious

Edit:
Actually I know, I broke the test, since it relied on rereading the info on every refresh

leomoty · 2023-09-08T15:44:15Z

Still a bit clueless on how to fix it, i'd expect an enter_page("/apps") after updating os-release to work, but doesn't sound like it.

diff --git a/test/verify/check-apps b/test/verify/check-apps
index a269d89a4..4de52e955 100755
--- a/test/verify/check-apps
+++ b/test/verify/check-apps
@@ -170,6 +170,7 @@ class TestApps(packagelib.PackageCase):
 
         # unknown OS: nothing gets installed
         m.write("/etc/os-release", 'ID="unmapped"\nID_LIKE="mysterious"\nVERSION_ID="1"\n')
+        b.enter_page("/apps")
         b.click("#refresh")
         # the progress bar is too fast to reliably catch it
         time.sleep(1)
@@ -178,6 +179,7 @@ class TestApps(packagelib.PackageCase):
 
         # known OS: appstream-data-tst gets installed from the map
         m.write("/etc/os-release", 'ID="derivative"\nID_LIKE="spicy testy"\nVERSION_ID="1"\n')
+        b.enter_page("/apps")
         b.click("#refresh")
         with b.wait_timeout(30):
             b.wait_visible(".app-list #app-1")

The first one seems to work, but if I were to guess it is just because of expecting the empty state

martinpitt · 2023-09-08T15:54:13Z

@leomoty sorry, this week was really hectic, we had an IRL team sprint. I'll review this on Monday.

test/verify/check-apps

martinpitt · 2023-09-08T15:56:16Z

I triggered https://cockpit-logs.us-east-1.linodeobjects.com/pull-19281-20230908-155558-64d3912b-fedora-38-other/log.html for now, to see how far it gets

leomoty · 2023-09-08T16:07:54Z

@leomoty sorry, this week was really hectic, we had an IRL team sprint. I'll review this on Monday.

I saw the group picture, pretty cool! Don't worry, thanks for putting up with me :)

pkg/apps/application-list.jsx

martinpitt · 2023-09-29T05:41:10Z

I split out the first two commits to PR #19416, as that should get fixed ASAP and there's more design bikeshedding here.

This gets added to `data` in the middle of `check_missing_packages()` anyway, so declare it right away. Also document it, as it's part of the result and e.g. `install_missing_packages()` assumes that this field exists.

martinpitt · 2023-10-11T12:30:55Z

Sorry, took me a while to get back to this, but I addressed @mvollmer 's feedback.

leomoty · 2023-10-11T12:46:53Z

Okay, now in retrospect I do understand what @mvollmer was asking, that makes sense.

mvollmer · 2023-10-12T07:58:54Z

pkg/apps/application-list.jsx

-                ? <EmptyStatePanel title={ _("No applications installed or available.") } />
+                ? <EmptyStatePanel title={ _("No applications installed or available.") }
+                                   paragraph={data_missing_msg}
+                                   action={_("Install application information")} onAction={refresh} />


Should this button always be here, even if data_missing_msg is null?

Ouch, thanks for spotting! Fixed, and added a test (which took a bit of work to test a negative)

mvollmer · 2023-10-12T08:03:37Z

Do we want to have some whitespace between the Alert and the Card?

martinpitt · 2023-10-12T08:51:50Z

Do we want to have some whitespace between the Alert and the Card?

Can't hurt, done. I updated the screenshot in the description. I also fixed the button issue you spotted above, and did some minor cleanups.

mvollmer · 2023-10-12T11:48:26Z

pkg/apps/application.scss

@@ -28,6 +28,10 @@
  overflow: hidden;
 }

+.ct-missing-metainfo-alert {
+  margin-block-end: 1rem;


Arg, CSS! :-) What about <Stack hasGutter>? But I am far from the authority here.

Thanks for the hint! Indeed that's nicer.

I needed to update this again anyway, I found that PackageKit behaved very poorly and blocked the check for a long time. I found out how to do that properly (lightning fast now, and also easier).

Introduce a new packagekit.js helper check_uninstalled_packages() which is a lightweight version of check_missing_packages() that avoids the expensive Refresh call. If there are no installed nor available apps, show an explanation and an action button in the empty state, otherwise show an alert with an "Install" button on top of the installed apps. Use the latter in TestApps.testBasic, as the top right "Refresh" button is already covered by other test cases. Fixes cockpit-project#18454 Thanks to leomoty <[email protected]> for the idea and initial implementation!

cockpituous · 2023-10-12T13:13:34Z

pkg/apps/application-list.jsx

+        } catch (e) {
+            console.warn("Failed to check missing AppStream metadata packages:", e.toString());


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

mvollmer

Thanks!

leomoty mentioned this pull request Sep 4, 2023

applications: Show more available applications #18454

Closed

leomoty changed the title ~~Application list may install appstream if missing~~ apps: Application list may install appstream if missing Sep 4, 2023

martinpitt requested changes Sep 4, 2023

View reviewed changes

pkg/apps/application-list.jsx Outdated Show resolved Hide resolved

pkg/apps/application-list.jsx Outdated Show resolved Hide resolved

pkg/apps/application-list.jsx Outdated Show resolved Hide resolved

leomoty force-pushed the apps-appstream-install branch from 631b973 to 637bccd Compare September 4, 2023 15:49

leomoty force-pushed the apps-appstream-install branch from 637bccd to b2e0897 Compare September 4, 2023 16:09

leomoty requested a review from martinpitt September 4, 2023 16:09

leomoty commented Sep 4, 2023

View reviewed changes

pkg/apps/application-list.jsx Outdated Show resolved Hide resolved

leomoty force-pushed the apps-appstream-install branch from b2e0897 to 914f7ee Compare September 4, 2023 16:19

leomoty force-pushed the apps-appstream-install branch 2 times, most recently from de7a136 to dca567b Compare September 4, 2023 22:19

martinpitt requested changes Sep 5, 2023

View reviewed changes

test/verify/check-apps Outdated Show resolved Hide resolved

test/verify/check-apps Outdated Show resolved Hide resolved

test/verify/check-apps Outdated Show resolved Hide resolved

martinpitt mentioned this pull request Sep 5, 2023

apps: Consider ID_LIKE for mapping AppStream data packages #19283

Merged

leomoty force-pushed the apps-appstream-install branch from dca567b to ef37084 Compare September 6, 2023 00:24

leomoty requested a review from martinpitt September 6, 2023 00:35

leomoty force-pushed the apps-appstream-install branch from 2dc5ba7 to 5d15a58 Compare September 6, 2023 00:39

leomoty changed the title ~~apps: Application list may install appstream if missing~~ apps: Warn if appstream data package is missing Sep 6, 2023

leomoty force-pushed the apps-appstream-install branch from 5d15a58 to 64d3912 Compare September 7, 2023 11:24

martinpitt reviewed Sep 8, 2023

View reviewed changes

test/verify/check-apps Outdated Show resolved Hide resolved

martinpitt requested changes Sep 8, 2023

View reviewed changes

pkg/apps/application-list.jsx Outdated Show resolved Hide resolved

martinpitt removed the blocked Don't land until something else happens first (see task list) label Sep 29, 2023

lib: Document and properly initialize packagekit missing_ids

077b43e

This gets added to `data` in the middle of `check_missing_packages()` anyway, so declare it right away. Also document it, as it's part of the result and e.g. `install_missing_packages()` assumes that this field exists.

martinpitt force-pushed the apps-appstream-install branch from 80b8161 to 09b9bc2 Compare October 11, 2023 12:29

martinpitt requested review from mvollmer and jelly October 11, 2023 13:06

jelly previously approved these changes Oct 12, 2023

View reviewed changes

mvollmer reviewed Oct 12, 2023

View reviewed changes

mvollmer self-requested a review October 12, 2023 08:03

martinpitt dismissed jelly’s stale review via a6f6f8f October 12, 2023 08:50

martinpitt force-pushed the apps-appstream-install branch from 09b9bc2 to a6f6f8f Compare October 12, 2023 08:50

martinpitt removed the request for review from mvollmer October 12, 2023 08:58

martinpitt mentioned this pull request Oct 12, 2023

testOverMounting fails with: Error synchronizing after initial wipe: Timed out waiting for object #19445

Closed

martinpitt requested a review from mvollmer October 12, 2023 10:39

mvollmer reviewed Oct 12, 2023

View reviewed changes

mvollmer previously approved these changes Oct 12, 2023

View reviewed changes

martinpitt dismissed mvollmer’s stale review via 1a910de October 12, 2023 12:05

martinpitt force-pushed the apps-appstream-install branch from a6f6f8f to 1a910de Compare October 12, 2023 12:05

martinpitt requested a review from mvollmer October 12, 2023 12:06

cockpituous reviewed Oct 12, 2023

View reviewed changes

mvollmer approved these changes Oct 13, 2023

View reviewed changes

martinpitt merged commit 57c930c into cockpit-project:main Oct 13, 2023
98 checks passed

martinpitt added the release-note label Oct 18, 2023

leomoty deleted the apps-appstream-install branch October 21, 2023 17:08

jelly removed the release-note label Nov 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apps: Warn if appstream data package is missing #19281

apps: Warn if appstream data package is missing #19281

leomoty commented Sep 4, 2023 •

edited by martinpitt

Loading

martinpitt left a comment

leomoty commented Sep 4, 2023

leomoty commented Sep 4, 2023

leomoty commented Sep 4, 2023

leomoty commented Sep 4, 2023 •

edited

Loading

leomoty commented Sep 4, 2023 •

edited

Loading

martinpitt left a comment

leomoty commented Sep 6, 2023

leomoty commented Sep 7, 2023 •

edited

Loading

leomoty commented Sep 8, 2023 •

edited

Loading

martinpitt commented Sep 8, 2023

martinpitt commented Sep 8, 2023

leomoty commented Sep 8, 2023

martinpitt commented Sep 29, 2023

martinpitt commented Oct 11, 2023

leomoty commented Oct 11, 2023

mvollmer Oct 12, 2023

martinpitt Oct 12, 2023

mvollmer commented Oct 12, 2023

martinpitt commented Oct 12, 2023

mvollmer Oct 12, 2023

martinpitt Oct 12, 2023

cockpituous Oct 12, 2023

mvollmer left a comment

		} catch (e) {
		console.warn("Failed to check missing AppStream metadata packages:", e.toString());

apps: Warn if appstream data package is missing #19281

apps: Warn if appstream data package is missing #19281

Conversation

leomoty commented Sep 4, 2023 • edited by martinpitt Loading

Apps: Detect missing AppStream metadata

martinpitt left a comment

Choose a reason for hiding this comment

leomoty commented Sep 4, 2023

leomoty commented Sep 4, 2023

leomoty commented Sep 4, 2023

leomoty commented Sep 4, 2023 • edited Loading

leomoty commented Sep 4, 2023 • edited Loading

martinpitt left a comment

Choose a reason for hiding this comment

leomoty commented Sep 6, 2023

leomoty commented Sep 7, 2023 • edited Loading

leomoty commented Sep 8, 2023 • edited Loading

martinpitt commented Sep 8, 2023

martinpitt commented Sep 8, 2023

leomoty commented Sep 8, 2023

martinpitt commented Sep 29, 2023

martinpitt commented Oct 11, 2023

leomoty commented Oct 11, 2023

mvollmer Oct 12, 2023

Choose a reason for hiding this comment

martinpitt Oct 12, 2023

Choose a reason for hiding this comment

mvollmer commented Oct 12, 2023

martinpitt commented Oct 12, 2023

mvollmer Oct 12, 2023

Choose a reason for hiding this comment

martinpitt Oct 12, 2023

Choose a reason for hiding this comment

cockpituous Oct 12, 2023

Choose a reason for hiding this comment

mvollmer left a comment

Choose a reason for hiding this comment

leomoty commented Sep 4, 2023 •

edited by martinpitt

Loading

leomoty commented Sep 4, 2023 •

edited

Loading

leomoty commented Sep 4, 2023 •

edited

Loading

leomoty commented Sep 7, 2023 •

edited

Loading

leomoty commented Sep 8, 2023 •

edited

Loading