storage: Install NFS packages on demand #8913

mvollmer · 2018-03-28T12:23:18Z

Testing

Currently, this just install the "nfs-utils" package. So to test, remove "nfs-utils" first.

Tasks

Make list of packages configurable
Say how many packages and how many bytes.
Test
Make sure everything is alright when PackageKit is not available
Implement mockups
Make sure this works as expected for non-root admins, and non-admins
Move "Checking installed software" into dialog

Demo for release notes: https://youtu.be/Gaioqm7sLEo

mvollmer · 2018-03-28T12:24:55Z

Super quick demo: https://www.youtube.com/watch?v=gpoF3cU-Y0U

mvollmer · 2018-03-28T12:26:05Z

@larskarlitski, fyi.

mvollmer · 2018-03-28T13:38:25Z

Say how many packages and how many bytes.

This probably need the DependsOn PackageKit method, which isn't supported for dnf, so yeah.

martinpitt · 2018-03-28T13:57:26Z

Nice video! I like that it is fairly unintrusive. Curious that you already have an NFS mount even before you install necessary software :-)

garrett · 2018-03-28T17:49:52Z

That's a great demo!

I wonder if having it that easy to install software might make some sysdamins feel a bit leery. 😉

martinpitt

Initial review. Thanks!

martinpitt · 2018-03-29T08:16:04Z

pkg/lib/packagekit.es6

+ * will take, but, PackageKit...
+ */
+
+export function check_optional_packages(names, progress_cb) {


"optional" is IMHO a bit confusing - we are using this for use cases where said packages are actually required. How about check_missing_packages()?

Yes, very good! I didn't like "optional" either.

martinpitt · 2018-03-29T08:16:28Z

pkg/lib/packagekit.es6

+ */
+
+export function check_optional_packages(names, progress_cb) {
+    if (names.length == 0)


martinpitt · 2018-03-29T08:16:47Z

pkg/lib/packagekit.es6

+                                      {
+                                          Package: (info, package_id) => {
+                                              console.log("P", info, package_id);
+                                              var repos = package_id.split(";")[3].split(":");


Don't use var in arrow functions (const or let), var has broken scoping.

Fixed. But is there something special with var in arrow functions? I think it's not anymore broken with arrow functions than with function functions.

martinpitt · 2018-03-29T08:19:47Z

pkg/lib/packagekit.es6

+                                              console.log("P", info, package_id);
+                                              var repos = package_id.split(";")[3].split(":");
+                                              if (repos.indexOf("installed") == -1)
+                                                  ids.push(package_id);


That's what your IRC ping from yesterday was about, right? Really weird that PK_FILTER_ENUM_NOT_INSTALLED does not work - what other purpose would that have? But this workaround looks simple enough.

martinpitt · 2018-03-29T08:25:56Z

pkg/lib/packagekit.es6

+    return refresh().then(resolve).then(get_deps);
+}
+
+export function install_optional_packages(data, progress_cb) {


This function does almost nothing on top of InstallPackages, so it feels a bit like noise. But I have no strong opinion if you want to keep this wrapper. But then please call it install_packages() and give it its own documentation comment.

martinpitt · 2018-03-29T08:27:27Z

pkg/lib/packagekit.es6

+
+export function check_optional_packages(names, progress_cb) {
+    if (names.length == 0)
+        return cockpit.resolve(null);


Please use/return standard Promise instead of cockpit.defer, as the rest of this library already does that.

martinpitt · 2018-03-29T08:28:34Z

pkg/lib/packagekit.es6

+                                          },
+                                      }).then(() => {
+                                          return data;
+                                      });


.then( () => data);

martinpitt · 2018-03-29T08:30:48Z

pkg/lib/packagekit.es6

+                                          if (ids.length == 0)
+                                              return null;
+                                          else
+                                              return { pkg_ids: ids };


Do you rely on the special null case in particular? It seems more consistent to always just return ids even if empty. install_packages() below in particular deals fine with this, and it also makes it easier to log/print/check the value without always having to consider the null special case.

I see the point, but with the current API, pkg_ids is internal and the client shouldn't access it. I had some public fields like package_count, but without a working DependsOn, it would be misleading to display "1 software package will be installed" when in fact we will install 20 (because of deps).

garrett · 2018-05-09T12:41:29Z

I've updated the mockup:

removed alternate
renumbered the list item as 0., so 1. can be the checking step
added dep checking in-dialog

Here's the relevant excerpt:

Concerns:

Dialog will have to be resized to show deps.
Screen readers may read things incorrectly. (Related: reading deps would be super-wordy...)

martinpitt

Nice work! I haven't tested it yet, just reviewed the code. I found a few issues.

martinpitt · 2018-05-09T12:28:47Z

pkg/lib/packagekit.es6

+                                      [ Enum.FILTER_ARCH |
+                                        Enum.FILTER_NOT_SOURCE |
+                                        Enum.FILTER_NEWEST,
+                                      names


indentation is off here (I wonder why eslint didn't spot that)

This is what eslint wants. It rejects the 'correct' indentation. Maybe it doesn't handle this style of multi-line arrays. I have put it all on one line, which fits now.

martinpitt · 2018-05-09T12:29:32Z

pkg/lib/packagekit.es6

+                                      {
+                                          Package: (info, package_id) => {
+                                              var parts = package_id.split(";");
+                                              var repos = parts[3].split(":");


var → let in arrow functions or nested scopes please; scoping of var doesn't go well with either. Same problem further down.

Fixed. What's wrong with varin arrow functions? It's as broken as with normal functions, but not more broken, right?

No, let has a scope of the enclosing { } block, i. e. what you intuitively expect. var's scope is always the innermost enclosing function (not arrow-closure!), so that if you declare a var in an arrow function, for loop, if block etc., it will still exist outside of that block. I. e. var other than directly in a function is conceptually broken and error prone, and should be avoided.

See https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Statements/let

martinpitt · 2018-05-09T12:33:07Z

pkg/lib/packagekit.es6

+                .then(() => {
+                    for (var i = 0; i < names.length; i++) {
+                        if (!installed_names[names[i]] && data.missing_names.indexOf(names[i]) == -1)
+                            data.unavailable_names.push(names[i]);


Instead of accessing names[i] several times, how about names.forEach(name => { ... })?

martinpitt · 2018-05-09T12:37:45Z

pkg/lib/cockpit-components-dialog.css

+#cockpit_modal_dialog .modal-body {
+    max-height: calc(75vh - 10rem);
+    overflow: auto;
+}


Scrolling the entire dialog body seems a bit odd; don't we want to just scroll the package list?

Hmm, @garrett?

martinpitt · 2018-05-09T12:38:43Z

pkg/lib/packagekit.es6

@@ -244,7 +244,8 @@ export function cancellableTransaction(method, arglist, progress_cb, signalHandl
                        // avoid calling progress_cb after ending the transaction, to avoid flickering cancel buttons
                        ErrorCode: (code, detail) => {
                            progress_cb = null;
-                            reject({ detail, code: cancelled ? "cancelled" : code });
+                            reject({ detail, code: cancelled ? "cancelled" : code,
+                                     toString: () => detail });


That's a nice trick, thanks!

It's obsolete now with the use of TransactionError, right?

martinpitt · 2018-05-09T12:42:18Z

pkg/storaged/client.js

+                },
+                function () {
+                    return PK.call("/org/freedesktop/PackageKit", "org.freedesktop.DBus.Properties",
+                                   "Get", [ "org.freedesktop.PackageKit", "VersionMajor" ]).then(


This is also a good candidate for moving into lib/packagekit. (Not a blocker for this PR)

It has been moved.

martinpitt · 2018-05-09T12:45:58Z

pkg/storaged/optional-panel.jsx

+    if (data.extra_names.length > 0 && data.remove_names.length == 0) {
+        summary = (
+            <p>
+                { format_to_array(_("$0 will be installed, along with additional packages:"), missing_names) }


This should use ngettext. Depending on if it's one or more packages, translation will be different in non-English languages; e. g. in German, "a wird installiert" vs. "a, b werden installiert". This also occurs a few times below.

martinpitt · 2018-05-09T12:51:12Z

test/verify/check-storage-nfs

+        # The first simulated install seems to silently not report
+        # anything on the Debian test images, for unknown reasons.  So
+        # we install a dummy package to warm up all parts of the
+        # machinery and distribute the fluids evenly.


This is a stupendously exact technical explanation what's going on 🤣

Could it perhaps be limited to if "debian" in m.image or "ubuntu" in m.image, to ensure we know when it happens elsewhere?

garrett · 2018-05-09T16:03:25Z

So I was thinking about this issue:

Dialog will have to be resized to show deps.

Having it jump with that information is not so nice.

Therefore, I figured out how to make it scale with a bit of CSS (and a tiny bit of JS to add a CSS class):

https://garrett.github.io/cockpit-mockweb/dialog-scale/

The idea is the scale would be triggered between step 1 and 2 (and not when clicking a button, like in the demo itself).

garrett · 2018-05-14T12:37:38Z

Oh! Oops; sorry. The modified version is actually in a different file:

mvollmer · 2018-05-17T09:08:05Z

There is now a new cockpit-component-install-dialog that encapsulates all the work.

The OptionalPanel isn't meant to be reusable anymore but I left the API as it is, without putting back the old assumptions of how we store our feature flags in the client. That's a bit more verbose, but the feature detection business can use some cleanup and this could be the first step.

By using "super()" instead of hardcoding the base class.

The cancel handler can now also be handed in via a progress report on the action promise. In addition, the Cancel button itself now is only enabled during an action when there is a cancel handler.

mvollmer · 2018-05-17T11:16:12Z

@martinpitt, ptal!

martinpitt

This works nicely, and the code now looks by and large good to me, too. Would be nice to fix the (apparently) broken commit message and add a "Closes #" to make this button-mergeable. @andreasn, do you want to review the design, too? You have a pending review.

martinpitt · 2018-05-17T12:54:07Z

pkg/lib/cockpit-components-install-dialog.jsx

+                })
+                .catch(error => {
+                    dfd.reject(error);
+                });


Note: should suffice to write just .then(dfd.resolve) and .catch(dfd.reject); but it's fine like that. Does the caller firmly expect a cockpit promise? A standard Promise doesn't work?

I am not convinced that the two forms are equivalent, because of this. With a call like dfd.resolve(), the resolve method will have this == dfd. With just .then(dfd.resolve), this is likely undefined. This might not matter depending on whether the resolve method actually uses this or not. I think it doesn't, but that's just luck. Am I wrong?

Ah, could be. We use that pattern a lot (git grep fail.*reject) and it seems to work, but indeed it could be that this is by luck.

martinpitt · 2018-05-17T12:55:00Z

pkg/lib/cockpit-components-install-dialog.css

@@ -0,0 +1,38 @@
+.package-list-ct {


The commit log looks like you got a phone call when starting to type it? :-) ("lib/cockpit-components-install-dialog: New ").

That was on purpose and how we did it back in the days. :-) What else do you expect for a completely new module?

Closes cockpit-project#8913

andreasn · 2018-05-21T10:02:05Z

Do you have a newer video of this? Because I'm having trouble getting it to run :(

martinpitt · 2018-05-22T06:04:31Z

@andreasn: The video in the description seems current, that's how it looks like for me.

andreasn

Good to go!

andreasn self-requested a review March 28, 2018 13:23

mvollmer force-pushed the storage-nfs-on-demand branch from c9f2e56 to 8459e1d Compare March 29, 2018 04:08

martinpitt reviewed Mar 29, 2018

View reviewed changes

mvollmer force-pushed the storage-nfs-on-demand branch 5 times, most recently from aa19d15 to ecd6592 Compare April 4, 2018 12:19

mvollmer changed the title ~~WIP - storage: Install NFS packages on demand~~ storage: Install NFS packages on demand Apr 4, 2018

mvollmer force-pushed the storage-nfs-on-demand branch from ecd6592 to 2b035cd Compare April 4, 2018 12:50

mvollmer force-pushed the storage-nfs-on-demand branch from 2b035cd to 86080b3 Compare April 4, 2018 13:16

mvollmer force-pushed the storage-nfs-on-demand branch from 86080b3 to 22edefc Compare April 5, 2018 11:03

mvollmer force-pushed the storage-nfs-on-demand branch from 22edefc to c196c03 Compare April 5, 2018 11:21

mvollmer force-pushed the storage-nfs-on-demand branch from c196c03 to 4d2d667 Compare April 5, 2018 11:29

mvollmer force-pushed the storage-nfs-on-demand branch from 4d2d667 to 4da7875 Compare April 9, 2018 14:49

mvollmer force-pushed the storage-nfs-on-demand branch from 4da7875 to dc74c93 Compare April 10, 2018 07:44

mvollmer force-pushed the storage-nfs-on-demand branch from dc74c93 to 93675a2 Compare April 10, 2018 11:30

mvollmer mentioned this pull request May 9, 2018

storage: Install VDO support on demand #9141

Merged

2 tasks

martinpitt requested changes May 9, 2018

View reviewed changes

mvollmer added the needswork label May 14, 2018

mvollmer force-pushed the storage-nfs-on-demand branch 4 times, most recently from 6eb7bda to e766996 Compare May 17, 2018 08:28

mvollmer added 7 commits May 17, 2018 13:47

lib/packagekit: Add API for checking and installing missing packages

eace614

lib/dialog: Restrict body height and make it scroll automatically

8c9cc3c

lib/dialog: Support static messages in the footer

82193e0

test: Allow StorageCase and PackageCase to be used together

b3c6fbc

By using "super()" instead of hardcoding the base class.

lib/packagekit: Add "detect" function

1b66779

lib/cockpit-components-dialog: Allow dynamic setting of cancel handler

dcb5212

The cancel handler can now also be handed in via a progress report on the action promise. In addition, the Cancel button itself now is only enabled during an action when there is a cancel handler.

lib/cockpit-components-install-dialog: New

c2f9281

mvollmer force-pushed the storage-nfs-on-demand branch from e766996 to d3e4cb2 Compare May 17, 2018 10:52

martinpitt reviewed May 17, 2018

View reviewed changes

storage: Allow installation of nfs client support

6111d7a

Closes cockpit-project#8913

mvollmer force-pushed the storage-nfs-on-demand branch from d3e4cb2 to 6111d7a Compare May 18, 2018 08:16

martinpitt removed the needswork label May 22, 2018

martinpitt approved these changes May 22, 2018

View reviewed changes

andreasn approved these changes May 22, 2018

View reviewed changes

martinpitt merged commit b529784 into cockpit-project:master May 22, 2018

mvollmer mentioned this pull request Aug 16, 2018

lib: Dialog body scrolling is optional #9832

Merged

storage: Install NFS packages on demand #8913

storage: Install NFS packages on demand #8913

Conversation

mvollmer commented Mar 28, 2018 • edited

Testing

Tasks

mvollmer commented Mar 28, 2018

mvollmer commented Mar 28, 2018

mvollmer commented Mar 28, 2018

martinpitt commented Mar 28, 2018

garrett commented Mar 28, 2018

martinpitt 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvollmer Mar 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrett commented May 9, 2018

martinpitt 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

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

garrett commented May 9, 2018

garrett commented May 14, 2018

mvollmer commented May 17, 2018

mvollmer commented May 17, 2018

martinpitt 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

andreasn commented May 21, 2018

martinpitt commented May 22, 2018

andreasn left a comment

Choose a reason for hiding this comment

mvollmer commented Mar 28, 2018 •

edited

mvollmer Mar 29, 2018 •

edited