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.

Sign up for GitHub

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

Jump to bottom

feat: added RawOutputDataConfig in ExecutionSpec #patch #329

Merged
merged 10 commits into from
Mar 18, 2022
Merged

feat: added RawOutputDataConfig in ExecutionSpec #patch #329

merged 10 commits into from
Mar 18, 2022

Conversation

pingsutw
Copy link
Member

Signed-off-by: Kevin Su [email protected]

TL;DR

added RawOutputDataConfig in ExecutionSpec, so we can override LaunchPlan's RawOutputDataConfig at CreateExecutionRequest time

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

image

Tracking Issue

flyteorg/flyte#2230

Follow-up issue

NA

@welcome
Copy link

welcome bot commented Mar 16, 2022

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).

@codecov
Copy link

codecov bot commented Mar 16, 2022 •
edited
Loading

Codecov Report

Merging #329 (5a8818d) into master (5732fc1) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master     #329      +/-   ##
==========================================
- Coverage   65.15%   65.14%   -0.02%     
==========================================
  Files         390      390              
  Lines        8705     8707       +2     
  Branches     1505     1506       +1     
==========================================
  Hits         5672     5672              
- Misses       3033     3035       +2
Impacted Files Coverage Δ
...cutions/ExecutionDetails/RelaunchExecutionForm.tsx 100.00% <ø> (ø)
src/components/Launch/LaunchForm/launchMachine.ts 86.53% <ø> (ø)
src/components/Launch/LaunchForm/types.ts 100.00% <ø> (ø)
...ts/Launch/LaunchForm/useLaunchWorkflowFormState.ts 98.87% <ø> (ø)
src/models/Common/types.ts 100.00% <ø> (ø)
src/models/Execution/api.ts 58.97% <0.00%> (-3.19%) ⬇️
...nts/Launch/LaunchForm/LaunchFormAdvancedInputs.tsx 82.25% <50.00%> (ø)
...utions/ExecutionDetails/ExecutionMetadataExtra.tsx 100.00% <100.00%> (ø)

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 5732fc1...5a8818d. Read the comment docs.

anrusina
anrusina reviewed Mar 16, 2022
View reviewed changes
Copy link
Contributor

@anrusina anrusina left a comment

Choose a reason for hiding this comment

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

Hi Kevin,
Can you please rebase on top of latest master. Today a big eslint/prettier changes were merged, which already took care of all spaces changes.
Unfortunately your PR was done before that - and as result, it is hard to follow which exactly changes were added by you, and which are just styling update.

Please, let me know if it's a hustle, and I will try to update it from my side (looks like you have your own fork of flyteconsole, which makes it a bit more complicated..).

@pingsutw
Merged master
931b2dc 
Signed-off-by: Kevin Su <[email protected]>
@pingsutw
Lint fixed
ff80038 
Signed-off-by: Kevin Su <[email protected]>
@pingsutw
Lint fixed
33ff020 
Signed-off-by: Kevin Su <[email protected]>
@pingsutw
Lint fixed
992e39a 
Signed-off-by: Kevin Su <[email protected]>
@pingsutw
Lint fixed
a06ac41 
Signed-off-by: Kevin Su <[email protected]>
@pingsutw
Lint fixed
c2f2eed 
Signed-off-by: Kevin Su <[email protected]>
@pingsutw
Lint fixed
ed1c3f3 
Signed-off-by: Kevin Su <[email protected]>
@pingsutw
Lint fixed
f5b0f4e 
Signed-off-by: Kevin Su <[email protected]>
@pingsutw
Lint fixed
5a8818d 
Signed-off-by: Kevin Su <[email protected]>
anrusina
anrusina reviewed Mar 17, 2022
View reviewed changes
package.json
@@ -82,7 +82,7 @@
"@commitlint/cli": "^8.3.5",
"@commitlint/config-conventional": "^8.3.4",
"@date-io/moment": "1.3.9",
"@flyteorg/flyteidl": "0.21.24",
"@flyteorg/flyteidl": "0.23.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Did smoke test, the new field looks good.
Is it expected, that if provided Config is not accessible it is using default value?
See here:
https://user-images.githubusercontent.com/55718143/158853593-7c554fba-b544-4017-9607-41fe7ddc1c4a.mp4
Otherwise LGTM, and I can merge it later.

Copy link
Member Author

@pingsutw pingsutw Mar 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

yes, We should use the default value if the bucket is not accessible

anrusina
anrusina approved these changes Mar 17, 2022
View reviewed changes
Copy link
Contributor

@anrusina anrusina left a comment

Choose a reason for hiding this comment

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

Lgtm

@anrusina anrusina changed the title feat: added RawOutputDataConfig in ExecutionSpec feat: added RawOutputDataConfig in ExecutionSpec #patch Mar 17, 2022
@anrusina anrusina merged commit 24bdaee into flyteorg:master Mar 18, 2022
@welcome
Copy link

welcome bot commented Mar 18, 2022

Congrats on merging your first pull request! 🎉

flyte-bot added a commit that referenced this pull request Mar 18, 2022
@flyte-bot
chore(release): Release 0.46.0 [skip ci]
1b3987a 
# [0.46.0](http://github.com/lyft/flyteconsole/compare/v0.45.0...v0.46.0) (2022-03-18)

### Features

* added RawOutputDataConfig in ExecutionSpec #patch ([#329](http://github.com/lyft/flyteconsole/issues/329)) ([24bdaee](http://github.com/lyft/flyteconsole/commit/24bdaeefcf32c079d1aaeb54b28cc6bf7395b42f))
@flyte-bot
Copy link
Collaborator

🎉 This PR is included in version 0.46.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

olga-union pushed a commit that referenced this pull request Mar 23, 2022
@pingsutw @olga-union
feat: added RawOutputDataConfig in ExecutionSpec #patch (#329)
732ff30 
* feat: added RawOutputDataConfig in ExecutionSpec
Signed-off-by: Kevin Su [email protected]

Signed-off-by: Olga Nad <[email protected]>
olga-union pushed a commit that referenced this pull request Mar 23, 2022
@flyte-bot @olga-union
chore(release): Release 0.46.0 [skip ci]
f842d2d 
# [0.46.0](http://github.com/lyft/flyteconsole/compare/v0.45.0...v0.46.0) (2022-03-18)

### Features

* added RawOutputDataConfig in ExecutionSpec #patch ([#329](http://github.com/lyft/flyteconsole/issues/329)) ([24bdaee](http://github.com/lyft/flyteconsole/commit/24bdaeefcf32c079d1aaeb54b28cc6bf7395b42f))

Signed-off-by: Olga Nad <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anrusina anrusina anrusina approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pingsutw @flyte-bot @anrusina