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

Ostree Software Updates #2884

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@petervo
Contributor

petervo commented Oct 1, 2015

To test, from inside the cockpit test directory run.

TEST_OS=fedora-atomic-22 ./testsuite-prepare

The generated image will have versions of ostree and rpm-ostree that work. It also contains a checked out copy of the current os at /var/local-tree and repo at /var/local-repo. You can run this image with vm-run and make changes to that tree and commit to that repo.

To make testing simplier there is a temporary ostree-run command.

./ostree-run

That will run the vm and gives you three commands

  • start-cockpit: Starts cockpit, needs to be run after every reboot.
  • start-httpd: Starts a http server on the local repo to allow you to pull updates. Until it is started you'll see errors when trying to check for updates. Doesn't persist between reboots.
  • prep-upgrade: This creates a new upgrade that installs a package, removes a package, upgrades a package and downgrades a package. This command should only be run once. If you want further updates you'll need to generate them yourself.

Because you'll probably be restarting the vm while testing running

virsh console id-from-ouput

will let you see when the machine has rebooted and is ready to accept commands again.

@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Oct 9, 2015

Contributor

Still needs a little bit of work, but ready for initial review. It should now be testable with the fedora-atomic-22 vm we use for testing.

Contributor

petervo commented Oct 9, 2015

Still needs a little bit of work, but ready for initial review. It should now be testable with the fedora-atomic-22 vm we use for testing.

@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Oct 13, 2015

Contributor

@stefwalter @andreasn, instructions for manual testing are added at the top. LMK if you have questions.

Contributor

petervo commented Oct 13, 2015

@stefwalter @andreasn, instructions for manual testing are added at the top. LMK if you have questions.

@stefwalter

This comment has been minimized.

Show comment
Hide comment
@stefwalter
Contributor

stefwalter commented Oct 13, 2015

@andreasn

This comment has been minimized.

Show comment
Hide comment
@andreasn

andreasn Oct 13, 2015

Contributor

The spacing in top of this error looks a bit off.

screenshot from 2015-10-13 23-04-15

Contributor

andreasn commented Oct 13, 2015

The spacing in top of this error looks a bit off.

screenshot from 2015-10-13 23-04-15

@andreasn

This comment has been minimized.

Show comment
Hide comment
@andreasn

andreasn Oct 13, 2015

Contributor

The listing of all packages is very long and easily pushes down the box below quite a lot.
How do you feel about giving listing-body the properties height: 200px and overflow-y: scroll; ?

Contributor

andreasn commented Oct 13, 2015

The listing of all packages is very long and easily pushes down the box below quite a lot.
How do you feel about giving listing-body the properties height: 200px and overflow-y: scroll; ?

@andreasn

This comment has been minimized.

Show comment
Hide comment
@andreasn

andreasn Oct 13, 2015

Contributor

The checking for updates text that appears briefly feels a bit twitchy. Maybe it just works to disable the button in that case since it appears so briefly.

Contributor

andreasn commented Oct 13, 2015

The checking for updates text that appears briefly feels a bit twitchy. Maybe it just works to disable the button in that case since it appears so briefly.

@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Oct 13, 2015

Contributor

On 10/13/2015 02:31 PM, andreasn wrote:

The checking for updates text that appears briefly feels a bit twitchy.
Maybe it just works to disable the button in that case since it appears
so briefly.


Reply to this email directly or view it on GitHub
#2884 (comment).

It's only like that if there is nothing to pull otherwise it looks good.
But of course we don't know that till after we pull. We can put in on a
delay but of course there is no guarantee that it won't finish right
after the delay expires. So not sure what the solution is.

Contributor

petervo commented Oct 13, 2015

On 10/13/2015 02:31 PM, andreasn wrote:

The checking for updates text that appears briefly feels a bit twitchy.
Maybe it just works to disable the button in that case since it appears
so briefly.


Reply to this email directly or view it on GitHub
#2884 (comment).

It's only like that if there is nothing to pull otherwise it looks good.
But of course we don't know that till after we pull. We can put in on a
delay but of course there is no guarantee that it won't finish right
after the delay expires. So not sure what the solution is.

@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Oct 13, 2015

Contributor

The listing of all packages is very long and easily pushes down the box
below quite a lot.
How do you feel about giving listing-body the properties height: 200px
and overflow-y: scroll; ?

@stefwalter, design specifically said not to add scrolling. WDYT?

Contributor

petervo commented Oct 13, 2015

The listing of all packages is very long and easily pushes down the box
below quite a lot.
How do you feel about giving listing-body the properties height: 200px
and overflow-y: scroll; ?

@stefwalter, design specifically said not to add scrolling. WDYT?

@andreasn

This comment has been minimized.

Show comment
Hide comment
@andreasn

andreasn Oct 16, 2015

Contributor

design specifically said not to add scrolling.

If that's by design, I'm OK with that.

Contributor

andreasn commented Oct 16, 2015

design specifically said not to add scrolling.

If that's by design, I'm OK with that.

@andreasn

This comment has been minimized.

Show comment
Hide comment
@andreasn

andreasn Oct 16, 2015

Contributor

We can put in on a delay but of course there is no guarantee that it won't finish right after the delay expires. So not sure what the solution is.

What if you put the message inside the listing-head div and make it go on the left of the box?

Contributor

andreasn commented Oct 16, 2015

We can put in on a delay but of course there is no guarantee that it won't finish right after the delay expires. So not sure what the solution is.

What if you put the message inside the listing-head div and make it go on the left of the box?

@andreasn

This comment has been minimized.

Show comment
Hide comment
@andreasn

andreasn Oct 16, 2015

Contributor

Played around with the HTML and css a bit.
If the error message is put inside the listing-head div and gets a css property of display: inline; it looks a bit more balanced.

screenshot from 2015-10-16 15-54-33

Contributor

andreasn commented Oct 16, 2015

Played around with the HTML and css a bit.
If the error message is put inside the listing-head div and gets a css property of display: inline; it looks a bit more balanced.

screenshot from 2015-10-16 15-54-33

Show outdated Hide outdated pkg/base1/cockpit.css
@@ -742,6 +742,20 @@ tbody.active .listing-head {
clear: right;
}
div.listing-maybe .listing-error {

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Could you add an example of how to use these styles to the playground/patterns.html?

@stefwalter

stefwalter Oct 20, 2015

Contributor

Could you add an example of how to use these styles to the playground/patterns.html?

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

And this is worth merging early as a separate pull request, so others can make use of it.

@stefwalter

stefwalter Oct 20, 2015

Contributor

And this is worth merging early as a separate pull request, so others can make use of it.

Show outdated Hide outdated pkg/ostree/app.js
var _ = cockpit.gettext;
var phantom_checkpoint = phantom_checkpoint || function () { };
function track(item) {

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

This function has a name that makes me think it does something different than what it does.

@stefwalter

stefwalter Oct 20, 2015

Contributor

This function has a name that makes me think it does something different than what it does.

function notify_result(promise, scope) {
promise
.progress(function(msg) {
scope.$applyAsync(function() {

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

applyAsync is nice. I didn't know it existed.

@stefwalter

stefwalter Oct 20, 2015

Contributor

applyAsync is nice. I didn't know it existed.

Show outdated Hide outdated pkg/ostree/app.js
templateUrl: 'index.html',
controller: 'indexController'
});
}])

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

The brace should move one line up to align with 'function'

@stefwalter

stefwalter Oct 20, 2015

Contributor

The brace should move one line up to align with 'function'

Show outdated Hide outdated pkg/ostree/app.js
function check_empty() {
if (timeout)
window.clearTimeout(timeout);

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

clearTimeout can be explicitly called with null, no need for the if.

@stefwalter

stefwalter Oct 20, 2015

Contributor

clearTimeout can be explicitly called with null, no need for the if.

Show outdated Hide outdated pkg/ostree/app.js
failure: ex,
message: message };
$scope.$digest();
phantom_checkpoint();

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Suggest implementing a helper function for doing these two together. Either that, or figure out a way to call phantom_checkpoint() globally for any digest.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Suggest implementing a helper function for doing these two together. Either that, or figure out a way to call phantom_checkpoint() globally for any digest.

Show outdated Hide outdated pkg/ostree/index.html
<script src="../base1/bundle.js"></script>
<script src="../ostree/ostree.js"></script>
</head>
<body>

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Body should be hidden until curtains != silent. Well either that, or just explictly show it without involving angularjs.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Body should be hidden until curtains != silent. Well either that, or just explictly show it without involving angularjs.

This comment has been minimized.

@petervo

petervo Oct 20, 2015

Contributor

Are you sure, kube doesn't do that. ng-cloak and the curtains logic is on the main div inside the body, everything else is in a script tag which won't show.

If we want to use angular on the body we probably have to move the mainController.

@petervo

petervo Oct 20, 2015

Contributor

Are you sure, kube doesn't do that. ng-cloak and the curtains logic is on the main div inside the body, everything else is in a script tag which won't show.

If we want to use angular on the body we probably have to move the mainController.

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Our index.html framing wants to see body appear when the page is ready. This leads to less flicker, and allows the framing spinner to work properly. So in general we should move towards doing it that way. Unless it's dumb and broken ... in which case we should change index.html and come up with a new pattern.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Our index.html framing wants to see body appear when the page is ready. This leads to less flicker, and allows the framing spinner to work properly. So in general we should move towards doing it that way. Unless it's dumb and broken ... in which case we should change index.html and come up with a new pattern.

Show outdated Hide outdated pkg/ostree/app.js
timeout = null;
})
.done(function(connection) {
$(client).on("connectionLost.main", function(event, ex) {

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

For multiple reconnects theseh event handlers will be connected multiple times. Why not just connect them once?

@stefwalter

stefwalter Oct 20, 2015

Contributor

For multiple reconnects theseh event handlers will be connected multiple times. Why not just connect them once?

$scope.$applyAsync(function() {
$scope.runningMethod = client.running_method;
$scope.os_list = client.os_list;

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

No phantom_checkpoint()?

@stefwalter

stefwalter Oct 20, 2015

Contributor

No phantom_checkpoint()?

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Should we just $scope.$watch(function() { phantom_checkpoint() }); ?

@stefwalter

stefwalter Oct 20, 2015

Contributor

Should we just $scope.$watch(function() { phantom_checkpoint() }); ?

Show outdated Hide outdated pkg/ostree/app.js
.controller('mainController', [
'$scope',
'$timeout',
'ostreeClient',

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Why not just use the client global?

@stefwalter

stefwalter Oct 20, 2015

Contributor

Why not just use the client global?

Show outdated Hide outdated pkg/ostree/app.js
}])
.factory('ostreeClient', [function() {
return client;

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Why not just use the client global? and leave this factory out?

@stefwalter

stefwalter Oct 20, 2015

Contributor

Why not just use the client global? and leave this factory out?

Show outdated Hide outdated pkg/ostree/app.js
}
])
.filter('packages', ["$filter", function() {

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Why is $filter needed here?

@stefwalter

stefwalter Oct 20, 2015

Contributor

Why is $filter needed here?

Show outdated Hide outdated pkg/ostree/app.js
};
}])
.filter('releaseName', ["$filter", function() {

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Why $filter?

@stefwalter

stefwalter Oct 20, 2015

Contributor

Why $filter?

Show outdated Hide outdated pkg/ostree/client.js
@@ -0,0 +1,513 @@
define(["jquery",

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Can we put these on the next line, indented 4 spaces?

@stefwalter

stefwalter Oct 20, 2015

Contributor

Can we put these on the next line, indented 4 spaces?

Show outdated Hide outdated pkg/ostree/client.js
define(["jquery",
"base1/cockpit",
], function($, cockpit) {
"use strict";

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Indentation.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Indentation.

Show outdated Hide outdated pkg/ostree/client.js
* Similar to the cli output but simplier.
* We don't display object counts or bytes/s.
* Percentages are only possible when
* we actually know what is going to be pulled

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Can we have a description of the structs/tuples involved in this comment?

@stefwalter

stefwalter Oct 20, 2015

Contributor

Can we have a description of the structs/tuples involved in this comment?

Show outdated Hide outdated pkg/ostree/client.js
});
}
function RPMOSTreeDbusClient() {

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Capital 'B'?

@stefwalter

stefwalter Oct 20, 2015

Contributor

Capital 'B'?

Show outdated Hide outdated pkg/ostree/client.js
};
}
return new RPMOSTreeDbusClient();

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

We haven't done this before. But now come to think of it, it's a great way to do a singleton without the nonsense we do in kubernetes/client.js

Are there any downsides that you know about?

@stefwalter

stefwalter Oct 20, 2015

Contributor

We haven't done this before. But now come to think of it, it's a great way to do a singleton without the nonsense we do in kubernetes/client.js

Are there any downsides that you know about?

This comment has been minimized.

@petervo

petervo Oct 20, 2015

Contributor

I couldn't think of any off the top of my head

@petervo

petervo Oct 20, 2015

Contributor

I couldn't think of any off the top of my head

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Can you comment that that this is a singleton, above the return.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Can you comment that that this is a singleton, above the return.

Show outdated Hide outdated pkg/ostree/index.html
<html>
<head>
<title>Software Updates</title>

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

translatable.

@stefwalter

stefwalter Oct 20, 2015

Contributor

translatable.

Show outdated Hide outdated pkg/ostree/index.html
<link href="../base1/cockpit.css" rel="stylesheet">
<link href="../ostree/ostree.css" rel="stylesheet">
<script src="../base1/bundle.js"></script>
<script src="../ostree/ostree.js"></script>

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

No need to go up, just ostree.js

@stefwalter

stefwalter Oct 20, 2015

Contributor

No need to go up, just ostree.js

Show outdated Hide outdated pkg/ostree/index.html
<title>Software Updates</title>
<meta charset="utf-8">
<link href="../base1/cockpit.css" rel="stylesheet">
<link href="../ostree/ostree.css" rel="stylesheet">

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

No need to go up, just ostree.css

@stefwalter

stefwalter Oct 20, 2015

Contributor

No need to go up, just ostree.css

Show outdated Hide outdated pkg/ostree/index.html
</script>
<script>
require(["base1/cockpit",

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Lets use the usual indentation style we see elsewhere for this.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Lets use the usual indentation style we see elsewhere for this.

INSTALL_RPMS = [
"empty-1-0.noarch",
"cockpit-shell-wip-0.1.noarch",
"cockpit-networkmanager-wip-2.noarch",

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Why are we using cockpit rpms here?

@stefwalter

stefwalter Oct 20, 2015

Contributor

Why are we using cockpit rpms here?

This comment has been minimized.

@petervo

petervo Oct 20, 2015

Contributor

Because that way we can pregenerate these empty RPMs with what will always be a greater and lesser version than what the base tree has installed and test for those changes. All other installed rpms the versions are subject to change depending on the exact version of the tree we happen to run the test against.

@petervo

petervo Oct 20, 2015

Contributor

Because that way we can pregenerate these empty RPMs with what will always be a greater and lesser version than what the base tree has installed and test for those changes. All other installed rpms the versions are subject to change depending on the exact version of the tree we happen to run the test against.

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Sounds fine.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Sounds fine.

Show outdated Hide outdated test/ostree-run
@@ -0,0 +1,29 @@
#!/usr/bin/python

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

This won't be merged right?

@stefwalter

stefwalter Oct 20, 2015

Contributor

This won't be merged right?

This comment has been minimized.

@petervo

petervo Oct 20, 2015

Contributor

Right, the commit this is part of is clearly marked to not be merged.

@petervo

petervo Oct 20, 2015

Contributor

Right, the commit this is part of is clearly marked to not be merged.

@@ -424,6 +427,14 @@ cluster. Installed on the Kubernetes master. This package is not yet complete.
%endif
%package ostree
Summary: Cockpit user interface for rpm-ostree

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Dependencies need to be added.

@stefwalter

stefwalter Oct 20, 2015

Contributor

Dependencies need to be added.

This comment has been minimized.

@petervo

petervo Oct 20, 2015

Contributor

I held off on doing this since the version we would need to depend on doesn't exist yet. Also adding the dependency will pull rpm-ostree into all our test systems, we probably need a skip mechanism on the other OSes.

On the other hand this packages is really only for atomic and you can't have an atomic system without rpm-ostree. Installing this packages without rpm-ostree just results in the getting the curtain message.

@petervo

petervo Oct 20, 2015

Contributor

I held off on doing this since the version we would need to depend on doesn't exist yet. Also adding the dependency will pull rpm-ostree into all our test systems, we probably need a skip mechanism on the other OSes.

On the other hand this packages is really only for atomic and you can't have an atomic system without rpm-ostree. Installing this packages without rpm-ostree just results in the getting the curtain message.

Show outdated Hide outdated pkg/ostree/ostree.css
padding-bottom: 10px;
text-overflow: ellipsis;
text-align: right;
float: right;

This comment has been minimized.

@stefwalter

stefwalter Oct 20, 2015

Contributor

indentation.

@stefwalter

stefwalter Oct 20, 2015

Contributor

indentation.

@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Oct 21, 2015

Contributor

@stefwalter addressed the rest of the review points.

@andreasn, i moved the check for updates progress and error messages to display-inline blocks like you asked. I'm not sure it really looks better esp on small screens but let me know what you think.

Contributor

petervo commented Oct 21, 2015

@stefwalter addressed the rest of the review points.

@andreasn, i moved the check for updates progress and error messages to display-inline blocks like you asked. I'm not sure it really looks better esp on small screens but let me know what you think.

@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Nov 6, 2015

Contributor

@stefwalter @andreasn The upstream rpm-ostree changes are in giving us the option to deploy a specific commit. I can change this PR so that it uses that new deploy command when upgrading to avoid potential races. Are there any other changes we want to make to the way this UI works before we get this in?

Contributor

petervo commented Nov 6, 2015

@stefwalter @andreasn The upstream rpm-ostree changes are in giving us the option to deploy a specific commit. I can change this PR so that it uses that new deploy command when upgrading to avoid potential races. Are there any other changes we want to make to the way this UI works before we get this in?

@stefwalter

This comment has been minimized.

Show comment
Hide comment
@stefwalter

stefwalter Nov 9, 2015

Contributor

Are there any other changes we want to make to the way this UI works before we get this in?

I think that was the only big missing thing. We can iterate further on the UI once it's in and people start using it.

Contributor

stefwalter commented Nov 9, 2015

Are there any other changes we want to make to the way this UI works before we get this in?

I think that was the only big missing thing. We can iterate further on the UI once it's in and people start using it.

@petervo petervo changed the title from WIP: ostree to Ostree Software Updates Nov 20, 2015

@petervo petervo added blocked and removed needswork labels Nov 20, 2015

@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Nov 20, 2015

Contributor

Updated to use the deploy command instead of upgrade, once we have a rpm-ostree rpm we can use this should be good to go.

Contributor

petervo commented Nov 20, 2015

Updated to use the deploy command instead of upgrade, once we have a rpm-ostree rpm we can use this should be good to go.

@stefwalter

This comment has been minimized.

Show comment
Hide comment
@stefwalter

stefwalter Nov 20, 2015

Contributor

@cgwalters When can we expect a new rpm-ostree release?

Contributor

stefwalter commented Nov 20, 2015

@cgwalters When can we expect a new rpm-ostree release?

@stefwalter

This comment has been minimized.

Show comment
Hide comment
@stefwalter

stefwalter Nov 23, 2015

Contributor

New upstream release is available. I guess we have to wait for it to be in a Fedora Atomic tree?

Contributor

stefwalter commented Nov 23, 2015

New upstream release is available. I guess we have to wait for it to be in a Fedora Atomic tree?

@dperpeet

This comment has been minimized.

Show comment
Hide comment
@dperpeet

dperpeet Dec 1, 2015

Member

Fixes #645

Member

dperpeet commented Dec 1, 2015

Fixes #645

@stefwalter stefwalter added needswork and removed blocked labels Dec 16, 2015

@petervo petervo added the blocked label Dec 17, 2015

@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Dec 17, 2015

Contributor

Depends on #3343 and new testing docker images to be deployed for the tests to pass.

Contributor

petervo commented Dec 17, 2015

Depends on #3343 and new testing docker images to be deployed for the tests to pass.

@stefwalter

This comment has been minimized.

Show comment
Hide comment
@stefwalter

stefwalter Dec 17, 2015

Contributor

We agreed this would be good to get in for the next release. So bumping priority.

Contributor

stefwalter commented Dec 17, 2015

We agreed this would be good to get in for the next release. So bumping priority.

@petervo petervo deleted the petervo:ostree branch Dec 17, 2015

sub-mod added a commit to sub-mod/cockpit that referenced this pull request Dec 20, 2015

ostree: UI for managing atomic host updates
Fixes #645
Closes #2884

Signed-off-by: Stef Walter <stefw@redhat.com>
 * Fixed merge conflict in test/vm-install
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment