-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conversation
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.
Ember Asset Size actionAs of 34e8e1f Files that got Bigger 🚨:
Files that stayed the same size 🤷:
|
Ember Test Audit comparison
|
Nice feature! 🥇 |
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 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.
ui/app/abilities/agent.js
Outdated
|
||
@computed('token.selfTokenPolicies.[]') | ||
get policiesIncludeAgentReadOrWrite() { | ||
const policies = (this.get('token.selfTokenPolicies') || []) |
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.
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) |
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 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); | ||
}, |
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 abstraction 🤩
.toString() | ||
.trim(), | ||
find('[data-test-output]').textContent.trim() | ||
); |
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 love it!!! Great test
@@ -1,5 +1,5 @@ | |||
{{title "Allocation " model.name}} | |||
{{allocation-subnav allocation=model}} | |||
<AllocationSubnav @allocation={{model}} /> |
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.
😳
|
||
@classic | ||
export default class ServerMonitorController extends Controller { | ||
queryParams = [{ level: 'level' }]; |
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 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 |
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 this can be de-@classic
-ed 🤓
ui/app/components/streaming-file.js
Outdated
@@ -41,8 +50,42 @@ export default class StreamingFile extends Component.extend(WindowResizable) { | |||
} | |||
} | |||
|
|||
scrollHandler() { | |||
const cli = this.element; | |||
window.requestAnimationFrame(() => { |
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.
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.
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.
Haaaaaaaa yep. Thank you for catching this.
if (currentTail) { | ||
currentTail += `\n...changing log level to ${this.level}...\n\n`; | ||
} | ||
this.set( |
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 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 😳
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. |
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 for making the changes! The documentation lists what @classic
is for but also this PR will help us too when it’s ready.
Does this work with the ACL subsystem disabled? |
It does, @analytically! I just tried it locally to confirm. The |
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. |
This adds
nomad monitor
support to the UI.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.
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).