Skip to content
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

Multiple Dashboards #1861

Closed
wants to merge 11 commits into from

Conversation

Projects
None yet
2 participants
@stefwalter
Copy link
Contributor

commented Feb 27, 2015

This does the navigation refactoring and redesign noted in #1726.

  • Move to a navigation more compatible with Patternfly
  • Multiple dashboards are now possible
  • The navbar and sidebar are implemented in pkg/shell/index.html and all the routing, loading of components, listing of machines for menus and related logic happens in pkg/shell/index.js
  • All server pages are loaded as components, even the ones in shell/shell.html
  • #hash URL formats change.

See below for behavioral change overview.

@stefwalter stefwalter force-pushed the stefwalter:dashboard-multi branch from c410250 to d7aa2cd Feb 27, 2015

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2015

Follow ups:

  • Better 404 error pages: #1865
  • Don't hard code components, tools, dashboards: #1859

For now the kubernetes dashboard will be hard coded in the top bar, but I would like to resolve #1859 next week and make it appear only when the kubernetes is installed.

@stefwalter stefwalter force-pushed the stefwalter:dashboard-multi branch from 6ea2121 to 71b312f Feb 27, 2015

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2015

Follow up:

  • Rework how machines are loaded: #1867

@stefwalter stefwalter force-pushed the stefwalter:dashboard-multi branch from 71b312f to 86bfc18 Feb 27, 2015

@stefwalter stefwalter changed the title WIP: Multiple Dashboards Multiple Dashboards Feb 27, 2015

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2015

This is ready for review. Integration tests still need to be updated.

@stefwalter stefwalter force-pushed the stefwalter:dashboard-multi branch from 86bfc18 to b32b5d5 Feb 27, 2015

@stefwalter stefwalter added this to the Modular Webapp milestone Feb 27, 2015

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2015

Design was done together with @andreasn and noted in #1726. A few tweaks were made during implementation, once it turned out that some of the ideas didn't work.

Screenshot tour:

When you first use Cockpit on a machine, you are taken to the system summary page. You can see the host name in the navbar along with the server's avatar.

screenshot from 2015-02-27 17 35 55

You can click on 'Dashboard' to go to the machines dashboard, where you can add another machine.

screenshot from 2015-02-27 17 38 19

Note that the host name now changes to a 'Machines' drop down. Once you have more than one machine in Cockpit, you can always use that menu to switch between machines. The colors and avatars that you see are those chosen in the dashboard (as you could before).

screenshot from 2015-02-27 17 39 02

Once you have more than one machine, you'll start to see the machine's color on the navbar to clearly give context when you select various machines. This machine has a blue color chosen.

screenshot from 2015-02-27 17 30 51

And here case we've selected another machine, which is tagged green. (ignore the "Not Found" here, the other machine didn't have the new package stuff installed).

screenshot from 2015-02-27 17 40 10

This Color + Host name + avatar change is extremely important. If you change between machines in the drop down, you remain in the same section on the new host (eg: storage -> storage) and we need to have clear indications the machine you are operating has changed. Thus the color + avatar change in the navbar.

Obviously, additional dashboards are available in the main navbar. I've hard coded the 'Cluster' link there for now, but soon this will be driven by addition and removal of the cockpit-kubernetes package. See #1859 for the work on that.

In the interim i've hard coded our kubernetes tool as a dashboard. This will soon be replaced by the real kubernetes dashboard in #1687.

screenshot from 2015-02-27 17 45 24

Oh, and once more than one machine is present, when you connect to Cockpit (without going to a specific link) you'll be taken by default to the main "Dashboard".

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2015

Tools drawer/expander work will come later: #1868

@stefwalter stefwalter force-pushed the stefwalter:dashboard-multi branch 2 times, most recently from cfa40ac to b232cb5 Feb 27, 2015

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2015

This requires reinstallation of cockpit (ie: make install) and restarting cockpit-ws. If you're using multiple machines, all the machines need to be updated.

This was referenced Mar 2, 2015

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2015

More follow ups:

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2015

  • Fixed 'Account Settings' menu item.
  • Pass options to frames
  • check-login passes

@stefwalter stefwalter force-pushed the stefwalter:dashboard-multi branch 2 times, most recently from 9f1191d to 184f24f Mar 2, 2015

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2015

All tests pass except for some check-storage races.

stefwalter added some commits Feb 25, 2015

ws: Redirect clients to checksum if we have one
If a checksum is available for a given host, then redirect clients
to it. This is usable both for embedding, but more importantly
for the package manifests. We only have to do one request, and we
can be redirected to the right version.
ws: Send ETags and respect If-None-Match header
We send ETags that match the server checksum, if one exists.

If we get a request with If-None-Match, then we check that it
matches the checksum in the URL, and return '304 Not Modified' if
that's the case.

In addition we can use the ETag header in XMLHttpRequset responses
to determine the checksum of a server, since XMLHttpRequest do
redirects transparently.
common: Don't send Content-Length for 304 HTTP responses
This is the 'Not Modified' error code.
base: Don't encode @ in hash part of URLs
We'll be using these a lot and we want it to be readable.

While '@' is problematic in other parts of an HTTP URL, it's ok
to have it in the hash part.
ws: Always prefer localhost as the place to retrieve resources
When accessing a resource via checksum, always try to load from
localhost first. This is for performance.
@mvollmer

This comment has been minimized.

Copy link
Member

commented Mar 2, 2015

#1876 should help with the storage races.

@stefwalter stefwalter removed the needswork label Mar 2, 2015

@mvollmer

This comment has been minimized.

Copy link
Member

commented Mar 3, 2015

check-roles tried to change the real name of "admin" but ended up changing the real name of "user" because the parameters of the page changed asynchronously from "account?id=admin" back to "account?id=user" during the test run, probably because of the relogin.

The sequence is something like this:

b.go("account?id=user")
b.enter_page("account")
b.relogin("account")
b.go("account?id=admin")
b.enter_page("account")            // was missing, but isn't enough
b.wait_text("#account-user-name", "admin")
// now we are on the "id=admin" page
// something happens in the background and we go back to "id=user"
b.set_val("#account-real-name", "Administratorrrr")

I could fix this like this:

diff --git a/test/check-roles b/test/check-roles
index 9f263c3..5da2710 100755
--- a/test/check-roles
+++ b/test/check-roles
@@ -85,9 +85,11 @@ class TestRoles(MachineCase):
         b.wait_text("#account-roles", "Server Administrator")

         b.relogin("account", "user")
+        b.wait_text("#account-user-name", "user")

         # Try to change admins name again
         b.go("account?id=admin")
+        b.enter_page("account")
         b.wait_text("#account-user-name", "admin")
         b.wait_present("#account-real-name:not([disabled])")
         b.wait_present("#account-locked:not([disabled])")

@stefwalter, does this make sense? (I'll go read the code now...)

@@ -1699,8 +1697,7 @@ PageContainerDetails.prototype = {
});
});

this.address = shell.get_page_machine();
this.client = shell.docker(this.address);
this.client = shell.docker(null);

This comment has been minimized.

Copy link
@mvollmer

mvollmer Mar 3, 2015

Member

Same.

@@ -2286,7 +2282,7 @@ function DockerClient(machine) {
});

/* TODO: This code needs to be migrated away from dbus-json1 */
dbus_client = shell.dbus(machine);
dbus_client = shell.dbus();

This comment has been minimized.

Copy link
@mvollmer

mvollmer Mar 3, 2015

Member

In other places, this is shell.dbus(null).

@stefwalter stefwalter force-pushed the stefwalter:dashboard-multi branch from 4960919 to 121fe4c Mar 3, 2015

stefwalter added a commit to stefwalter/cockpit that referenced this pull request Mar 3, 2015

test: Fix tests for new navigation
This includes changes to account for dashboards and everything
being in a frame.

Fixes cockpit-project#1726
Closes cockpit-project#1861
@@ -1409,8 +1408,7 @@ PageSearchImage.prototype = {
},

enter: function() {
this.address = shell.get_page_machine();
this.client = shell.docker(this.address);
this.client = shell.docker(null);

This comment has been minimized.

Copy link
@mvollmer

mvollmer Mar 3, 2015

Member

shell.docker doesn't take any arguments anymore.

@stefwalter stefwalter force-pushed the stefwalter:dashboard-multi branch from 121fe4c to fa5af8b Mar 3, 2015

stefwalter added a commit to stefwalter/cockpit that referenced this pull request Mar 3, 2015

test: Fix tests for new navigation
This includes changes to account for dashboards and everything
being in a frame.

Fixes cockpit-project#1726
Closes cockpit-project#1861
@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2015

Fixed 2 shell.docker(null) issues.

@mvollmer

This comment has been minimized.

Copy link
Member

commented Mar 3, 2015

Reloading a page appends /dashboard/dashboard/list to the URL, which leads to a "Not Found" error. Reloading a couple of times adds more dashboard strings.
screenshot from 2015-03-03 11 59 47

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2015

Fix for adding too many dashboard/list

-                cockpit.location.go("dashboard/list");
+                cockpit.location.go("/dashboard/list");

@stefwalter stefwalter force-pushed the stefwalter:dashboard-multi branch from fa5af8b to 8d0dc4f Mar 3, 2015

stefwalter added a commit to stefwalter/cockpit that referenced this pull request Mar 3, 2015

test: Fix tests for new navigation
This includes changes to account for dashboards and everything
being in a frame.

Fixes cockpit-project#1726
Closes cockpit-project#1861
@mvollmer

This comment has been minimized.

Copy link
Member

commented Mar 3, 2015

The components for a remote machine are actually showing stuff for localhost.

Note that this should show "Dandelion" as the hostname but shows "Sunflower", which is localhost.

screenshot from 2015-03-03 12 06 10

@stefwalter stefwalter force-pushed the stefwalter:dashboard-multi branch from 8d0dc4f to f6ef4e4 Mar 3, 2015

stefwalter added a commit to stefwalter/cockpit that referenced this pull request Mar 3, 2015

test: Fix tests for new navigation
This includes changes to account for dashboards and everything
being in a frame.

Fixes cockpit-project#1726
Closes cockpit-project#1861
@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2015

Fixed issue where reloading would go to dashboard. The navigate() function was being called before local machines were loaded.

Fixed issue where shell.html was connecting to wrong machines for shell.dbus(null) calls.

stefwalter added some commits Feb 27, 2015

shell: Implement new multiple dashboards navigation
This implements the multiple dashboards navigation that we've
been discussing.

I wish it was easier to split this up into multiple commits, but
it's all pretty intertwined.

The new index.html and index.js are the default page that's loaded.
Everything in shell.html is loaded as a framed component, along with
the things that are already components. We remove a bunch of code
related to machines in the shell javascript code.
pkg: Add new cockpit.jump() API
This enables telling the outer shell, to navigate between components.
shell: Use cockpit.jump() to move between components
And use target="_parent" where appropriate, in the dashboard.
kubernetes: Hard code Kubernetes as a dashboard for now
Hard code Kubernetes as a dashboard for now, until we have proper
declaration of dashboards in manifests.
test: Fix tests for new navigation
This includes changes to account for dashboards and everything
being in a frame.

Fixes #1726
Closes #1861
base: If a channel host field is empty null or undefined use default
Previously we would only use the default if the host field was
undefined. However this too fragile. It bit us in the shell component

@stefwalter stefwalter force-pushed the stefwalter:dashboard-multi branch from f6ef4e4 to b2e443d Mar 3, 2015

@mvollmer

This comment has been minimized.

Copy link
Member

commented Mar 3, 2015

Reloading loses subnavigation, but I think that's OK for now. (E.g., navigate to a network interface, reload, you are back at the network overview.)

@mvollmer

This comment has been minimized.

Copy link
Member

commented Mar 3, 2015

Fixed issue where reloading would go to dashboard. The navigate() function was being called before
local machines were loaded.

Works.

Fixed issue where shell.html was connecting to wrong machines for shell.dbus(null) calls.

Works.

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2015

Reloading loses subnavigation, but I think that's OK for now. (E.g., navigate to a network interface, reload, you are back at the network overview.)

This is only the case on shell legacy pages. And yes, that's an unfortunate limitation, but we should be migrating away from that.

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2015

@stefwalter stefwalter closed this in 22eecf9 Mar 3, 2015

@stefwalter stefwalter deleted the stefwalter:dashboard-multi branch Mar 3, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.