Add interruptible override to launch forms #chore

[MorpheusXAUT]

TL;DR

Adds an option to override the interruptible flag for a single execution to launch forms.

Type

• Bug Fix
• Feature
• Plugin

Are all requirements met?

• Code completed
• Smoke tested
• Unit tests added
• Code documentation added
• Any pending items have an associated Issue

Complete description

A new LaunchInterruptibleInput component has been added, providing a small form input for marking a single execution of a task or workflow as interruptible via a checkbox. Both LaunchTaskForm and LaunchWorkflowForm now include this additional input - it has been added to the advanced options of a workflow launch form.
Relaunching an execution applies the previously selected interruptible override setting to the new launch form.
Additionally, a simple true/false indication has been added to the execution metadata overview, displaying whether an execution had its interruptible flag overwritten.

Some very basic tests have been added to check correct application of the checkbox' value. Manual smoke testing for workflow and single task executions has been performed together with flightadmin and flightpropeller.

Tracking Issue

flyteorg/flyte#2284

Follow-up issue

NA

[welcome]

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

• Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
• Sign off your commits (Reference: DCO Guide).

[MorpheusXAUT]

Looks like checks failed due to unreleased flyteidl changes. Is there anything that should be done before the IDL changes are merged (like with replace in go.mod), or should I update my PR once the spec changes are available?

[anrusina]

Looks like checks failed due to unreleased flyteidl changes. Is there anything that should be done before the IDL changes are merged (like with replace in go.mod), or should I update my PR once the spec changes are available?

@MorpheusXAUT Any fix I can think of will fix it only locally for you, and will still present the problem in PR circle CI and during/after merge if your curernt PR depends on flyteidl changes.

We are using flyteidl changes in flyteconsole only to resolve typings, so if new interruptible parameter already exists in latest flyteidl, you can update package.json file with new version:

        "@flyteorg/flyteidl": "0.24.11",

and run yarn install locally afterwards.
Then add yarn.lock and package.json changes to this PR.

I should be able to smoke test changes and look deeper into in in few hours.

[MorpheusXAUT]

@MorpheusXAUT Any fix I can think of will fix it only locally for you, and will still present the problem in PR circle CI and during/after merge if your curernt PR depends on flyteidl changes.

Right, that's what I did for development locally as well...
Unfortunately, I've only created the PR for flyteidl today, so it's not been merged yet: flyteorg/flyteidl#287

@anrusina I'll update this issue once the changes have been approved/released and I can update the packages accordingly, sorry for the early PR in that case 😄

[MorpheusXAUT]

@anrusina Unfortunately still WIP due to pending flyteidl merges, but a slight change has been made already. As the interruptible flag is optional and supports an override to enable and disable the setting for an execution, I'm having to visualize a boolean with 3 states 😄
For now, I've come up with the following:

default, no override

override to enabled

override to disabled

Clicking the checkbox cycles through not set -> enabled -> disabled -> not set, updating the label for additional clarity.

As I'm neither a great frontend developer nor an UX expert, I'm not sure if there's a better way to represent this while being more clear. I could of course add two separate checkboxes/toggles a la "Enable override" and "Override to", however I found that a bit... tedious to use.

Is there a better/preferred way you can think of to visualize this new setting?

[anrusina]

Clicking the checkbox cycles through not set -> enabled -> disabled -> not set, updating the label for additional clarity.
As I'm neither a great frontend developer nor an UX expert, I'm not sure if there's a better way to represent this while being more clear. I could of course add two separate checkboxes/toggles a la "Enable override" and "Override to", however I found that a bit... tedious to use.

@MorpheusXAUT I don't think that two toggles will make it more clear, and solution you come up with is pretty clever. I asked our designer to look into it - to see if he can suggest something better in this case

[anrusina]

Beautiful code 😍, left two nits which could be ignored.
I definitely haven't smoked it yet, as flyteidl changes I believe still need some time to cook..
Package.k=json will need to point to new flyteidl version when it would be ready.

I will check tomorrow with our designer if he had any suggestions on UI change for you :)

[hongxunjin]

tedious

@MorpheusXAUT (cc @anrusina ) Overall, it is awesome! One thing that could possibly be improved is to set the color of the text [interruptible(no override)] as grey when the checkbox turns to "not set", which can visually differentiate "not set" and "disabled".

[MorpheusXAUT]

@MorpheusXAUT (cc @anrusina) Overall, it is awesome! One thing that could possibly be improved is to set the color of the text [interruptible(no override)] as grey when the checkbox turns to "not set", which can visually differentiate "not set" and "disabled".

Sounds like a good plan! How do you like the following (just setting color="textSecondary", or would you like a more nuanced color?)

indeterminate state

checked state

[hongxunjin]

indeterminate

It looks great!

[MorpheusXAUT]

Feedback from the review has been included, I'll push another commit and update this issue once the IDL changes are merged 🙂

[MorpheusXAUT]

@anrusina flyteidl/npm package has been published, PR should build locally/in CI now 🥳
Sorry for the delay!

[codecov]

Codecov Report

Merging #417 (12b8bff) into master (ddcda48) will increase coverage by 0.10%.
The diff coverage is 95.58%.

@@            Coverage Diff             @@
##           master     #417      +/-   ##
==========================================
+ Coverage   67.08%   67.18%   +0.10%     
==========================================
  Files         407      413       +6     
  Lines        9147     9295     +148     
  Branches     1614     1630      +16     
==========================================
+ Hits         6136     6245     +109     
- Misses       3011     3050      +39     

Impacted Files	Coverage Δ
...cutions/ExecutionDetails/RelaunchExecutionForm.tsx	100.00% <ø> (ø)
src/components/Launch/LaunchForm/constants.ts	100.00% <ø> (ø)
src/components/Launch/LaunchForm/launchMachine.ts	86.53% <ø> (ø)
src/components/Launch/LaunchForm/types.ts	100.00% <ø> (ø)
src/models/Execution/api.ts	58.97% <33.33%> (-3.19%)	⬇️
...nts/Launch/LaunchForm/LaunchInterruptibleInput.tsx	97.87% <97.87%> (ø)
...utions/ExecutionDetails/ExecutionMetadataExtra.tsx	100.00% <100.00%> (ø)
...omponents/Executions/ExecutionDetails/constants.ts	100.00% <100.00%> (ø)
...rc/components/Launch/LaunchForm/LaunchTaskForm.tsx	100.00% <100.00%> (ø)
...omponents/Launch/LaunchForm/LaunchWorkflowForm.tsx	100.00% <100.00%> (ø)
... and 20 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f2e13d...12b8bff. Read the comment docs.

[anrusina]

LGTM, some tests are needed, than could be merged.

[anrusina]

Lgtm

🚢

[welcome]

Congrats on merging your first pull request! 🎉