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

WebUI: Cockpit integration #927

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@pvomacka
Copy link
Contributor

commented Jul 21, 2017

Link to the cockpit is placed to each host details page in case
the Cockpit is installed on the server.

Showing or hiding cockpit link is possible because of Cockpit's API
which provides public URL for check whether Cockpit is the software
which listen on given port.

https://pagure.io/freeipa/issue/4891

@pvomacka

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2017

Do we want to also add a link on each line to the host Search facet?

Only consequence which would come with adding links into search facet is that there will be one AJAX call for each line and as the length of table is configurable it can lead to let's say 200 calls.

@pvomacka pvomacka requested a review from pvoborni Jul 24, 2017

@pvoborni pvoborni added the re-run label Aug 24, 2017

@freeipa-pr-ci freeipa-pr-ci removed the re-run label Aug 24, 2017

@pvoborni pvoborni added the re-run label Aug 30, 2017

@freeipa-pr-ci freeipa-pr-ci removed the re-run label Aug 30, 2017

@pvomacka pvomacka force-pushed the pvomacka:t4891 branch from 1787840 to 6fab6ab Aug 31, 2017

@pvoborni

This comment has been minimized.

Copy link
Member

commented Aug 31, 2017

  1. I'd change message Cockpit has not been detected on host.example.test host into just: Cockpit has not been detected on host.example.test

  2. When cockpit is detected the message should match it: e.g. MD equivalent: Cockpit detected on host. [Open Cockpit](https://host.example.test:9090)

  3. link_widget has on_link_clicked method. If you click on cockpit link this method is executed and fails with exception which is logged in console. Practically it won't be noticed much because the page is then switched to cockpit interface. But even though I think it should be fixed. It doesn't happen if cockpit UI is opened in new tab.

@pvomacka pvomacka force-pushed the pvomacka:t4891 branch from 6fab6ab to 86e25ec Sep 7, 2017

@abbra

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2017

@pvomacka could you please rebase this PR?

WebUI: Cockpit integration
Link to the cockpit is placed to each host details page in case
the Cockpit is installed on the server.

Showing or hiding cockpit link is possible because of Cockpit's API
which provides public URL for check whether Cockpit is the software
which listen on given port.

https://pagure.io/freeipa/issue/4891

@pvomacka pvomacka force-pushed the pvomacka:t4891 branch from 86e25ec to 0b1cf0d Oct 4, 2017

@pvomacka

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2017

@abbra done

@abbra

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2017

LGTM.

@abbra

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2017

So I tested it on a real system. Couple comments:

First, we should not automatically probe cockpit on each host page. This is not going to work because in many cases hosts might not be actually reachable from the client where browser runs.

Second, I'd probably change the whole section "Cockpit" to be an accordion. Such accordion would perform Cockpit ping on expand and would show a link to cockpit web UI if the ping is succeeding. This way we would only do a ping if user is interested in expanding Cockpit section.

For the case when ping is not succeeding, one way is to say that cockpit on the target host is unreachable or not installed.

Finally, when cockpit is reachable and a link can be provided, it would be good to pre-populate user name based on the current logged-in user in FreeIPA Web UI, if that's possible from cockpit side. TODO: check with cockpit developers.

@pvomacka

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2017

Hello @abbra,
thank you for the review.

Adding accordion just adds one more necessary click (that might make UX worse) to check whether the Cockpit is installed/reachable or not. Then, after clicking on it, we still won't be able to recognize whether the Cockpit is unreachable or is not installed. And one additional AJAX call costs almost nothing.

I would vote for extend the message, that the Cockpit is not installed or unreachable.

I agree that it might be useful to prefill the username field on Cockpit Login Page. I will check with them.

@abbra

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2017

Adding accordion just adds one more necessary click (that might make UX worse) to check whether the Cockpit is installed/reachable or not. Then, after clicking on it, we still won't be able to recognize whether the Cockpit is unreachable or is not installed. And one additional AJAX call costs almost nothing.

I disagree with this assessment. First, I did run this on a system that is relatively far away from my browser client (~40ms) and the cost of AJAX call was noticeable in the current implementation. If you aren't going to touch Cockpit every time you look at the host entry, for every host entry, it quickly becomes annoying. This is very bad UX in itself.

If you do need to touch Cockpit from IPA web UI, then unfolding a section is enough to trigger the AJAX call and generation of the link. There is nothing that prevents you to put AJAX call on the unfold event, so there is no degradation of user experience because for the case of not using Cockpit link a user doesn't have to witness AJAX call effect.

Can we also convert this feature into a Web UI plugin? It would be a good example how to extend existing Web UI code with something relatively complex (including communication to a separate web service). We may supply it as a sub-package too, to allow installing it separately and may be even require a cockpit on IPA master within the subpackage.

@tiran

This comment has been minimized.

Copy link
Member

commented Dec 14, 2017

@pvomacka The PR has been stuck for a while. Please rebase and address @abbra comment.

@pvomacka pvomacka added the postponed label Dec 15, 2017

@tiran tiran added rejected and removed postponed labels Feb 7, 2018

@tiran

This comment has been minimized.

Copy link
Member

commented Feb 7, 2018

We decided to provide cockpit integration as a separate plugin and not part of freeIPA upstream.

@tiran tiran closed this Feb 7, 2018

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.