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

UI: Client and Server Monitor #8177

Merged
merged 21 commits into from
Jun 18, 2020
Merged

UI: Client and Server Monitor #8177

merged 21 commits into from
Jun 18, 2020

Conversation

DingoEatingFuzz
Copy link
Contributor

This adds nomad monitor support to the UI.

image

image

As a consequence of adding the Servers > Server > Monitor route, the server detail page had to be redesigned to accommodate a subnav. I don't think anyone will miss the old design.

image

image

Along with these changes come one bug fix and one new feature that affects all the places where we stream output like this (task logs and file system viewer).

  • Bug Fix: The output window will actually follow your output now. It used to be that if you scrolled to the bottom of the output window and more output came in, sometimes it followed but most of the time it did not. This should always work now.
  • New Feature: Using cmd+a or ctrl+a to select all will now only select the text within the output window. This makes it much easier to copy logs and share them around.

These are based on the source code for selectChoose. I would have liked
to have used selectChoose, but the implementation has two await
settled()s in it which prevented me from writing the tests I needed to
write.

These new extension helpers separate selectChoose into two pieces so
logic can be placed between the two async actions.
@DingoEatingFuzz DingoEatingFuzz added this to the 0.12.0 milestone Jun 16, 2020
@DingoEatingFuzz DingoEatingFuzz requested review from backspace and a team June 16, 2020 20:03
@github-actions
Copy link

github-actions bot commented Jun 16, 2020

Ember Asset Size action

As of 34e8e1f

Files that got Bigger 🚨:

File raw gzip
nomad-ui.js +24.3 kB +2.26 kB
nomad-ui.css +361 B +48 B

Files that stayed the same size 🤷‍:

File raw gzip
auto-import-fastboot.js 0 B 0 B
vendor.js 0 B 0 B
vendor.css 0 B 0 B

@github-actions
Copy link

github-actions bot commented Jun 16, 2020

Ember Test Audit comparison

master 34e8e1f change
passes 1354 1391 +37
failures 0 0 0
flaky 0 1 +1
duration 5m 21s 642ms 5m 26s 505ms +04s 863ms

@rkettelerij
Copy link
Contributor

Nice feature! 🥇

Copy link
Contributor

@backspace backspace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran this locally, looks great. I didn’t exercise the ACL-checking, though. I appreciate the tests 😍

I left some minor comments, I guess requestFrame is an important one unless I’m misunderstanding.


@computed('token.selfTokenPolicies.[]')
get policiesIncludeAgentReadOrWrite() {
const policies = (this.get('token.selfTokenPolicies') || [])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also use get instead of this.get? I’m semi-surprised it doesn’t warn about this but maybe that’s because AbstractAbility is already @classic. (ETA I have since found it’s because I didn’t include the ESLint rules 😞)

@@ -0,0 +1,82 @@
import { click, settled } from '@ember/test-helpers';
// TODO: Contribute these helpers back upstream once we are on the latest version of
// ember-power-select (4.x)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope they accept! It always saddens me to run into this kind of thing.

const contentId = await selectOpen('[data-test-level-switcher]');
run.later(run, run.cancelTimers, 500);
await selectOpenChoose(contentId, level);
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice abstraction 🤩

.toString()
.trim(),
find('[data-test-output]').textContent.trim()
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love it!!! Great test

@@ -1,5 +1,5 @@
{{title "Allocation " model.name}}
{{allocation-subnav allocation=model}}
<AllocationSubnav @allocation={{model}} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😳


@classic
export default class ServerMonitorController extends Controller {
queryParams = [{ level: 'level' }];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn’t really matter but I think this could be ['level'], the object format I think is only needed when relabeling a query parameter? Also maybe this doesn’t need to be @classic.

import Controller from '@ember/controller';
import classic from 'ember-classic-decorator';

@classic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be de-@classic-ed 🤓

@@ -41,8 +50,42 @@ export default class StreamingFile extends Component.extend(WindowResizable) {
}
}

scrollHandler() {
const cli = this.element;
window.requestAnimationFrame(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be wrapped in an if to check on the value of requestFrame? It looks like the value of that field is updated but never checked.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haaaaaaaa yep. Thank you for catching this.

if (currentTail) {
currentTail += `\n...changing log level to ${this.level}...\n\n`;
}
this.set(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that using this.set means this needs to be @classic. I see now that I failed to follow the part of the instructions that would cause this to be a linting error, so I’m sorry about that, I’ll open a PR 😳

@DingoEatingFuzz
Copy link
Contributor Author

Thanks for the review @backspace! Everything should be addressed now. I should also read more about the classic decorator so I actually know when I am and am not supposed to use it.

Copy link
Contributor

@backspace backspace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the changes! The documentation lists what @classic is for but also this PR will help us too when it’s ready.

@DingoEatingFuzz DingoEatingFuzz merged commit 50cc295 into master Jun 18, 2020
@DingoEatingFuzz DingoEatingFuzz deleted the f-ui/monitor branch June 18, 2020 16:07
@analytically
Copy link

Does this work with the ACL subsystem disabled?

@backspace
Copy link
Contributor

It does, @analytically! I just tried it locally to confirm. The bypassAuthorization property is what causes permissions to be ignored if ACLs aren’t turned on. But let us know if you have any trouble? 💞

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants