-
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
packagekit: Add OS package update support #6724
Conversation
9f16a08
to
bae270d
Compare
Pushed initial basic tests, to see how far they get on all our images. Some of them don't have PackageKit installed and thus will have to be updated, and there might be other OS/release specific issues that I haven't considered/encountered yet. |
We need that to test OS updates (cockpit-project#6724) and soon for AppStream based service installation. Closes cockpit-project#6736
@garrett: The "installing updates" screen is currently a bit bare-bone, but the only reliable pieces of information that we have are the currently processed package, and the overall percentage: Is this okay for the first version, or do you have some suggestions about the design/layout here? |
1 similar comment
@garrett: I implemented some parts of the new design: the width-limited progress bar, no inline label any more, spelled out action label (Updating/Verifying/Downloading etc.), showing version number. if packagekit gives a time estimate, it is now shown on the right hand side too, but as dnf doesn't, it's not currently visible: Some issues I'd still like to discuss with you:
|
Splitting out the code into a new cockpit-pkgupdates binary package now requires adding PackageKit to at least Fedora-24 as well, for the selenium tests. I'll wait for the other test results and will then send a separate PR for that. |
The commit message has a duplicated paragraph "Set priority to 0 ...". |
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.
This is amazing. Needs a few fixes ... but really great work.
tools/cockpit.spec
Outdated
@@ -93,6 +93,7 @@ Recommends: %{name}-docker = %{version}-%{release} | |||
Suggests: %{name}-pcp = %{version}-%{release} | |||
Suggests: %{name}-kubernetes = %{version}-%{release} | |||
Suggests: %{name}-selinux = %{version}-%{release} | |||
Suggests: %{name}-pkgupdates = %{version}-%{release} |
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.
Are we sure about this name? Do you think we should make it be cockpit-packagekit ... to follow along with cockpit-ostree ... and with the assumption that other ways of updating a system will show up?
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.
I don't like cockpit-packagekit TBH:
- This is soon going to interact with dnf/yum/apt specific configuration for setting up automatic updates (as PackageKit doesn't cover that).
- PackageKit is not solely intended for updates, but also for finding and installing new software. For example, the app install bits that @mvollmer is working on is also going to use PK. PK is a means to an end, not a functionality description.
- If we are going to add other mechanisms to actually update packages (like calling yum directly instead of PackageKit), these would go into this module, not a different one.
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.
Okay that makes sense. But still not an trivial decision.
Our packages typically have not been named after functionality but after technology. So that multiple things with competing or different technologies can cover the same functionality ... and thus share a package alias (the "name"
field in manifest.json).
If we are going to add other mechanisms to actually update packages (like calling yum directly instead of PackageKit), these would go into this module, not a different one.
So does cockpit-ostree fould into this package or naming structure somehow? Or how does that fit into this picture? It's the same functionality with a different implementation.
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.
Right, ostree's manifest name is "updates", so it should probably have been called "ostree-updates", and we could call this "packagekit-updates" or "pk-updates" for brevity. To compare, we wouldn't put the functionality of searching and installing new apps (what @mvollmer is working on) into ostree just because that's also going to work on ostree.
Naming packages entirely after the underlying interfaces that they use is tricky -- for example, lots of our code talks to systemd, not only pkg/systemd.
So, I'm not sure about the package name for this, I just have a strong feeling that "cockpit-packagekit" is really misleading and ambiguous, and it should be something else. Meeting topic?
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.
Sure. Names a hard ... maybe writing a Feature page will help you figure that out.
https://github.com/cockpit-project/cockpit/tree/master/doc/guide
The feature pages are meant to help admins imagine how Cockpit is interacting with their system, and to give them clues on how to perform similar actions from the command line.
Once you think such a page through, maybe that'll help with figuring out a name. The package name should have a similar intent. Point toward the thing that we're interacting with, aid in diagnosis, quickly understanding how we're talking to the system.
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.
For the scope of this PR, the CLI equivalent is little more than "pkcon refresh" and "pkcon update" (I'll create a page for it). It's going to grow with configuring automatic updates and reboot detection though, as these need backend specific (yum/dnf/apt) commands. But I suppose the main conflict here is "stories/workflow" (apply/configure operating system updates) vs. "backend" (packagekit/dnf/apt/yum).
Makefile.am
Outdated
@@ -152,6 +152,7 @@ WEBPACK_PACKAGES = \ | |||
networkmanager \ | |||
ostree \ | |||
pcp \ | |||
pkgupdates \ |
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.
See comment below about the package name. Likely 'packagekit'?
pkg/pkgupdates/index.html
Outdated
<link href="../base1/cockpit.css" rel="stylesheet"> | ||
<link href="pkgupdates.css" rel="stylesheet"> | ||
|
||
<script src="../base1/jquery.js"></script> |
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.
Are you actually using jquery? If not, no need to include it.
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.
I do use it for setting D-Bus signal handlers with $(myDbusProxy).on(...)
. The JSX already require()s
it, but without this <script>
here I get an Oops about "ReferenceError: jQuery is not defined".
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.
In that case, I would suggest just using .addEventListener()
and removing all use of jQuery.
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.
Nice! Done. Worth updating http://cockpit-project.org/guide/latest/cockpit-dbus.html for that? (In a separate PR)
pkg/pkgupdates/index.html
Outdated
<meta charset="utf-8"> | ||
|
||
<link href="../base1/patternfly.css" type="text/css" rel="stylesheet"> | ||
<link href="../base1/cockpit.css" rel="stylesheet"> |
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.
Are you actually using cockpit.css? Typically all the css can be webpack bundled into pkgupdates.css rather than using this legacy file.
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.
Yes I do, mostly the table.listing-ct
bits.
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.
Then you should require("listing.less");
and don't worry about including this CSS file. This is legacy. We should use "static-linking" for all non-patternfly CSS stuff.
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.
Thanks, that works nicely. Fixed.
pkg/pkgupdates/index.html
Outdated
<html> | ||
|
||
<head> | ||
<title translate>Software Updates</title> |
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.
Does this actually get translated at runtime?
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.
I'm not sure how to verify that (i. e. how to create a PO template and update the translations). kubernetes, ostree, and systemd do the same, so I copied it from that.
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.
I think you just need to set the window.title at runtime during start up of your react based app.
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.
We don't set window.title
anywhere in Cockpit. It's not really necessary either, as even without it the window title on http://localhost:9090/updates is fine, it's apparently built from the manifest's tools→updates→label plus the hostname. Do you know if setting it explicitly is useful for anything? Perhaps embedding as iframe in third-party pages or so? As every other pkg/* has a <title>
I'm a bit hesitant to drop it.
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.
You don't need to drop the title tag. Just make sure it gets translated. It's marked for translation ... just make sure it gets translated in your app.
For example:
window.title = cockpit.gettext(window.title)
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.
This is the only translatable string outside of your React app. It makes sense to handle it properly.
Other apps that put everything in their HTML, use something like:
cockpit.translate()
to find and translate all these strings. I don't think you need that for this one string.
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.
That makes perfect sense, thanks for the explanation. Fixed.
pkg/pkgupdates/pkgupdates.jsx
Outdated
} | ||
}); | ||
|
||
|
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.
Extra line.
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.
fixed
pkg/pkgupdates/pkgupdates.jsx
Outdated
// return type is 'u', but returns -1 if not supported; so ignore implausibly high values (> 10 years) | ||
dbus_pk.call('/org/freedesktop/PackageKit', 'org.freedesktop.PackageKit', 'GetTimeSinceAction', | ||
[PK_ROLE_ENUM_REFRESH_CACHE], {timeout: 5000}) | ||
.done(seconds => (seconds > 315360000 || this.setState({timeSinceRefresh: seconds}))) |
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.
This magic number here could use a constant.
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.
Fixed.
test/verify/check-pkgupdates
Outdated
from testlib import * | ||
|
||
@skipImage("Image uses OSTree", "continuous-atomic", "fedora-atomic", "rhel-atomic") | ||
@skipImage("PackageKit crashes, https://launchpad.net/bugs/1689820", "ubuntu-1604") |
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.
This should be a known issue, not a skip.
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.
It needs to be a skip as currently cockpit-pkgupdates isn't built at all for ubuntu-1604 due to this bug. (See below).
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.
It can't build or just isn't building right now? We should build it even if an operating system bug prevents it from working ... and track it as a known issue.
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.
It could build, but I added blacklisting to tools/debian/rules. I really disagree about shipping it in a known completely broken state. I'm fine with building it on ubuntu-1604 for our CI, but not on "the real thing" (on releases and in Ubuntu xenial-backports).
test/verify/check-pkgupdates
Outdated
if self.machine.image in ["ubuntu-1604", "debian-stable"]: | ||
# old PackageKit+NM on Debian/Ubuntu misdetect online status with ifupdown; work around | ||
# https://launchpad.net/bugs/1694438 | ||
# TODO: use a naughty override instead? |
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.
Naughty override if it affects real users using the feature in Cockpit.
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.
Can do, but this means that we wouldn't test OS updates on debian-stable at all. It wouldn't affect debian-stable users who use ifupdown (i. e. servers), or use NetworkManager without a static ifupdown config (i. e. desktops). This just affects the case of ifupdown plus NM that we use on our test images. So I thought it would be more useful to run the checks with a config that works than to skip them entirely due to our special setup.
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.
Makes sense, if it doesn't affect most users, it's fine to work around it.
@@ -8,6 +8,12 @@ ifneq ($(shell dpkg -s libpcp3-dev >/dev/null 2>&1 && echo yes),yes) | |||
CONFIG_OPTIONS = --disable-pcp | |||
endif | |||
|
|||
# PackageKit crashes on update information on Ubuntu 16.04, which makes | |||
# "Software Updates" useless (LP: #1689820) |
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.
This is not a reason not to ship the feature. That's a bug in Ubuntu. And we should track it as a known naughty issue.
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.
Sure it is a bug in Ubuntu, and I filed it properly downstream. But this is a dealbreaker - I didn't find a workaround, and not being able to get updates makes this page entirely useless. I am not going to ship a completely broken new package, we can enable it later if/once the bug gets fixed. Note that pkgupdates does get shipped on ubuntu 17.04, just not on 16.04.
Some background: For a long time Ubuntu has used aptdaemon as a PackageKit compatible (-ish) implementation as PackageKit's apt support was rather poor in the past. In newer versions PackageKit itself has gotten much more attention, and Ubuntu moves away from aptdaemon.
pkg/pkgupdates/pkgupdates.jsx
Outdated
loadUpdateDetails(pkg_ids) { | ||
pkTransaction() | ||
.done(transProxy => { | ||
$(transProxy).on('UpdateDetail', (event, packageId, updates, obsoletes, vendor_urls, |
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.
This should just use transProxy.addEventListener('UpdateDetail', ... )
so we can drop jQuery. jQuery is really a legacy thing and not necessary in React apps.
This also needs a feature page in the documentation: https://github.com/cockpit-project/cockpit/tree/master/doc/guide |
I pushed the FIXUPs for @stefwalter's review comments, except for "add feature page" and "discuss package name" which I added to the TODO list in the description for now. |
Feature page pushed too now, CI was happy with the previous FIXUPs. |
pkg/pkgupdates/pkgupdates.jsx
Outdated
const PK_INFO_ENUM_SECURITY = 8; | ||
const PK_STATUS_ENUM_WAIT = 1; | ||
const PK_STATUS_ENUM_UPDATE = 10; | ||
const PK_STATUS_ENUM_WAITING_FOR_LOCK = 1; |
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.
PK_STATUS_ENUM_WAITING_FOR_LOCK is 30.
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.
Fixed locally (didn't push yet), thanks!
pkg/pkgupdates/pkgupdates.jsx
Outdated
const PK_STATUS_ENUM_WAITING_FOR_LOCK = 1; | ||
|
||
const PK_STATUS_STRINGS = { | ||
9: _("Downloading"), |
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.
PK_STATUS_ENUM_DOWNLOAD is 8, no?
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.
Dito, thanks!
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.
I have never seen the "Downloading" state, actually. I tried to provoke it, but failed. Maybe this is a PackageKit quirk on Fedora.
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.
Yes, me neither, the download part is reported as INSTALL
on Fedora. But I think it can't hurt to provide a human readable string for it anyway - it may get fixed in PackageKit one day, or already be used with other backends (yum, apt, etc.)
pkg/pkgupdates/pkgupdates.jsx
Outdated
|
||
case "loadError": | ||
case "updateError": | ||
return this.state.errorMessages.map(m => <pre>{m}</pre>); |
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.
There is no way out of this state except by reloading the page. I think there should be a way to go back to 'loading'.
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.
I'm not sure - if the upgrade fails, you should really investigate on the command line, as there is no interactivity in the PackageKit upgrade. The user might try to re-run the update, but that might make things worse (or at least not better). I'm okay with adding some "Acknowledge" or "Go back" button there in case a retry does help. @garrett, do you have some input on that?
I added it as a TODO item for now, but I don't think this necessarily needs to block this PR.
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.
I added it as a TODO item for now, but I don't think this necessarily needs to block this PR.
I agree.
I figure what's changed is that this time you only have one update with a very short description, no bugs etc. On "design patterns" the listing-ct isn't full width either, and it would only make the table harder to read to add extra whitespace. I wouldn't like to divert from cockpit's standard listing-ct here. I have 40 updates (since Friday) and the table is full width, with line breaks. |
Makes sense, thanks. |
@mvollmer: In today's weekly meeting we discussed the package name again, between "cockpit-pkgupdates" and "cockpit-packagekit". Do you have some thoughts how this would relate to your upcoming "app discovery/install" page? That will use PackageKit, docker, and (on Atomic) possibly OSTree? Does it make sense for it to depend on "cockpit-packagekit" or even go into that same binary package? How about the module name in |
@stefwalter: I agree. I didn't design an empty state yet — but definitely should. Thanks for pointing that out! 👍 |
cockpit-pkgupdates gets stuck on |
@da2x: I can easily reproduce the F26 PackageKit refresh crash, that was introduced only recently. This corresponds to the progress bar getting stuck at some 20% indeed. This doesn't send out an I now have a FIXUP with corresponding test case that handles PackageKit crashes. I will push it in a bit as I also change the "empty state" page according to @garrett 's designs and don't want to trigger CI too often. |
I made some empty state mockups, shared them with @martinpitt over IRC today, and we refined them iteratively (mainly for the current lack of auto-updates). ...and realized we should have a stale state, which automatically triggers a check for new packages if someone is visiting the page and there hasn't been a check in a while. (In the mockup, I have this at >= 1 day.) The available packages header is a little awkward here — especially when there are no auto-updates, so @martinpitt and I decided to drop the header altogether in these states for now. We'll revisit the header (and the text it may contain... if it should be different) when the automatic updates feature is added in. |
In IRC, @martinpitt mentioned that the update progress is actually known, so I'll look into modifying these mockups. They'll probably change inline to the latest versions, as I hotlinked them from their master version in the design repo. |
Add a "Software Updates" page for classic dnf/yum/apt based systems, to complement what "ostree" provides on Atomic. This uses the PackageKit API for platform independence. Set priority 0 to hide behind ostree if it is installed (which has default priority 1), so that we don't try to mess around with packages on OSTree/Atomic. Ship it in a new cockpit-packagekit rpm/deb as we don't want to pull in the packagekit dependency in cockpit-system (at least just yet). Don't ship this on Ubuntu 16.04, as a PackageKit crash there (https://launchpad.net/bugs/1689820) makes this functionality mostly useless. Specification: https://github.com/cockpit-project/cockpit/wiki/Feature:-System-Updates-for-dnf,-yum,-apt-hosts Closes cockpit-project#6724
Force-pushed again to rename from "pkgupdates" to "packagekit" as per team discussion. I also squashed all the fixups together. |
@mvollmer, @stefwalter : This is now all green and should address all your feedback. |
Add a "Software Updates" page for classic dnf/yum/apt based systems,
to complement what "ostree" provides on Atomic. This uses the PackageKit
API for platform independence.
Set priority 0 to "hide" behind ostree if it is installed (which has
default priority 1), so that we don't try to mess around with packages
on OSTree/Atomic. Ship it in a new cockpit-packagekit rpm/deb as we
don't want to pull in the packagekit dependency in cockpit-system (at
least just yet).
Don't ship this on Ubuntu 16.04, as a PackageKit crash there
(https://launchpad.net/bugs/1689820) makes this functionality mostly
useless.
Specification:
https://github.com/cockpit-project/cockpit/wiki/Feature:-System-Updates-for-dnf,-yum,-apt-hosts
This is work in progress mostly due to missing tests. This has existed as a standalone project and got a lot of fixes and some third-party testing already, so it now becomes time to integrate it properly. I'd appreciate an initial coarse-grained review to point out code style and similar structural flaws, as this is by and large my first piece of React work.
componentDidMount()
to button actionShouldn't block this PR, can be a followup:
<h2>
, the rest is implemented)