ui: task lifecycle restart all tasks

[lgfa29]

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:

[github-actions]

Ember Asset Size action

As of 566a150

Files that got Bigger 🚨:

File	raw	gzip
nomad-ui.js	+1.05 kB	+89 B

Files that stayed the same size 🤷‍:

File	raw	gzip
vendor.js	0 B	0 B
nomad-ui.css	0 B	0 B
vendor.css	0 B	0 B

[ChaiWithJai]

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!

[ChaiWithJai]

question: Why is this a boolean?

[lgfa29]

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 😅

[ChaiWithJai]

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?

[lgfa29]

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 😅

[github-actions]

Ember Test Audit comparison

	main	566a150	change
passes	1410	1410	0
failures	0	0	0
flaky	0	0	0
duration	11m 23s 643ms	9m 48s 789ms	-1m 34s 854ms

[philrenaud]

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.

[lgfa29]

Ahh good point on the restart function changes @ChaiWithJai and @philrenaud. You both made the same comment so I'm combining the discussion here 😄

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 restartAll() function sounds good. Using a special string "all" seems dangerous since people could have a task called all and then 💥.

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 restartAll, unless there's a better option?

EDIT:
Another option would be passing a config object, though I don't like it very much:

restart(allocation, {taskName, restartAll}) {
  // ...
}

[github-actions]

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.