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

Resume Signal Form #622

Merged
merged 18 commits into from
Oct 25, 2022
Merged

Resume Signal Form #622

merged 18 commits into from
Oct 25, 2022

Conversation

james-union
Copy link
Contributor

@james-union james-union commented Oct 21, 2022

https://github.com/flyteorg/flyteconsole/issues/587

TL;DR

  • SetSignal is still waiting to be deployed on the backend side

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

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

fixes https://github.com/flyteorg/flyteconsole/issues/587

@james-union james-union changed the base branch from master to devmain October 21, 2022 13:58
@james-union james-union self-assigned this Oct 21, 2022
Copy link
Contributor

@olga-union olga-union left a comment

Choose a reason for hiding this comment

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

Left some clean-up feedback, but haven't smoke tested yet.
One more item to think of - we use 3 different names for launch form trigger resumeOnClick, resumeAction, handleActionClick, let's pick one (maybe a different one) and stick to it, to improve search-ability of these blocks in future

Copy link
Contributor

@olga-union olga-union left a comment

Choose a reason for hiding this comment

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

Don't forget to include flyteidl update in this PR - ignore for now, looks like v1.1.22 doesn't have gate nodes

Copy link
Contributor

@olga-union olga-union left a comment

Choose a reason for hiding this comment

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

Almost there - few nits in the comments, let's move string from constants.ts file to a newly created localization strings.ts (more info in unresolved comments) in the LaunchForm folder, and rename resumeAction/resumeOnClick/handleActionClick with the same name!

Copy link
Contributor

@olga-union olga-union left a comment

Choose a reason for hiding this comment

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

Nice job! Few tiny things left, and don't forget to checkout package.jsons you accidentally committed 😃

@codecov
Copy link

codecov bot commented Oct 25, 2022

Codecov Report

Merging #622 (578943d) into devmain (bc2dd8e) will increase coverage by 0.21%.
The diff coverage is 74.02%.

@@             Coverage Diff             @@
##           devmain     #622      +/-   ##
===========================================
+ Coverage    67.35%   67.57%   +0.21%     
===========================================
  Files          398      447      +49     
  Lines         9296    10648    +1352     
  Branches      1590     1831     +241     
===========================================
+ Hits          6261     7195     +934     
- Misses        3035     3453     +418     
Impacted Files Coverage Δ
...zapp/console/src/basics/FeatureFlags/AdminFlag.tsx 94.11% <ø> (ø)
...p/console/src/basics/FeatureFlags/FeatureFlags.tsx 93.54% <ø> (ø)
...ges/zapp/console/src/basics/FeatureFlags/index.tsx 100.00% <ø> (ø)
packages/zapp/console/src/common/env.ts 100.00% <ø> (ø)
packages/zapp/console/src/common/layout.ts 100.00% <ø> (ø)
packages/zapp/console/src/common/linkify.ts 100.00% <ø> (ø)
packages/zapp/console/src/common/log.ts 100.00% <ø> (ø)
packages/zapp/console/src/common/promiseUtils.ts 0.00% <ø> (ø)
packages/zapp/console/src/common/timer.ts 0.00% <ø> (ø)
packages/zapp/console/src/common/timezone.ts 100.00% <ø> (ø)
... and 404 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: James <[email protected]>
@james-union
Copy link
Contributor Author

@olga-union I've fixed your comments. Just FYI, the flyteidl version is not yet updated because if it points to 1.1.14, it fails to build as Core.SignalIdentifier is not defined.
For the tests, I've fixed PausedTasksComponent & NodeExecutionActions

Still working on ResumeSignalForm

@jsonporter jsonporter merged commit be9f643 into devmain Oct 25, 2022
@jsonporter jsonporter deleted the james/gate-node-launch-form branch October 25, 2022 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] The plugins workqueue seem to be processing the same items over and over again
3 participants