-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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: task lifecycle restart all tasks #14223
Conversation
Ember Asset Size actionAs of 566a150 Files that got Bigger 🚨:
Files that stayed the same size 🤷:
|
8b813fd
to
c56cfdb
Compare
290b871
to
1425ad0
Compare
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.
LGTM. I think I understand why you decided to edit to restart
API to have a 3rd argument instead of refactoring the taskName
logic. However, I wonder if there was another option out there that would be more maintainable.
Great job nonetheless!
ui/app/adapters/allocation.js
Outdated
data.TaskName = taskName; | ||
} | ||
if (allTasks) { | ||
data.AllTasks = !!allTasks; |
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.
question
: Why is this a boolean?
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.
This API expects a boolean: https://github.com/hashicorp/nomad/pull/14127/files#diff-b4fa614bde4cb3604e2b5a1b95d00e4381a06e6e1cb1a29f2c34fe44a0444d01R468
Is there a better way to make sure a param is a boolean? That's how I used to do it 😅
try { | ||
yield this.model.restart(); | ||
yield this.model.restart('', allTasks); |
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.
question
thought
: You're passing in an empty string as a first argument here because we're looking to restart all tasks instead of only selecting one. That strikes me as a code smell for an API that should be refactored. But, I'm betting this is a backported change and we don't want to be risky like that.
Do you have any commentary about this so I can learn more?
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.
The API takes a body with either
{
"Task": "my-task"
}
Or
{
"AllTasks": true
}
If you try to set both it will result in an error. I didn't think too much about the changes to the function signatures but you're right, it doesn't look good with a random empty string 😅
Ember Test Audit comparison
|
ui/app/adapters/allocation.js
Outdated
@@ -6,12 +6,17 @@ import classic from 'ember-classic-decorator'; | |||
export default class AllocationAdapter extends Watchable { | |||
stop = adapterAction('/stop'); | |||
|
|||
restart(allocation, taskName) { | |||
restart(allocation, taskName, allTasks) { |
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 was going to suggest using taskName
as the param you could optionally pass "all"
to, to prevent you having to use a third parameter here, but I'm not sure if taskName can be arbitrary / one might call their task "all" in some scenario.
Either way: this seems like something worth breaking out into its own function like restartAll()
rather than adding into the restart function.
Ahh good point on the I didn't give too much thought about it, so a new param seemed like the easier option. But your comments make sense, I think I was sill with Go in mind where things are strongly typed, but in JS this is indeed handled differently. A new Another option would be to overload the second argument and then do a type check in the function, so something like: restart(allocation, taskNameOrAll) {
data = {}
if (typeof taskNameOrAll === 'string') {
data.TaskName = taskNameOrAll
} else {
data.AllTasks = !!taskNameOrAll
}
// ...
} I've seen this used in JS a few times, but I don't particularly like it 😅 I think I will go with the EDIT: restart(allocation, {taskName, restartAll}) {
// ...
} |
1c13d5d
to
0274b4f
Compare
Now that tasks that have finished running can be restarted, the UI needs to use the actual task state to determine which CSS class to use when rendering the task lifecycle chart element.
0274b4f
to
566a150
Compare
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 PR adds the option to restart all tasks in an allocation and fixes a CSS rule that was defined by a task
finishedAt
value. Since tasks that finished running can now be restarted, this check caused the lifecycle element to be rendered with the wrong class.After/Before:
![image](https://user-images.githubusercontent.com/775380/186030207-1850a05d-913f-4015-86f2-2040003b0ff1.png)