Add createworkflow failure proto #minor #331

jerempy · 2022-10-17T19:04:34Z

TL;DR

Adds createworkflow failure proto. This is for more concrete return values when a workflow fails to get created.
For context: flyteorg/flyteadmin#487 (comment)

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

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

Tracking Issue

Remove the 'fixes' keyword if there will be multiple PRs to fix the linked issue

flyteorg/flyte#2522

Follow-up issue

NA
OR
https://github.com/flyteorg/flyte/issues/

welcome · 2022-10-17T19:04:38Z

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

jerempy · 2022-10-17T19:12:29Z

@katrogan Would you mind taking a look at this? If it looks good to you, would you mind helping me compile the protobufs? This is out of my wheelhouse unfortunately 😞

katrogan · 2022-10-17T20:11:45Z

protos/flyteidl/core/workflow.proto

+
+// The workflow id is already used and the structure is different
+message WorkflowErrorExistsDifferentStructure {
+    string id = 1;


let's use the workflow identifier, like here: https://github.com/flyteorg/flyteidl/blob/master/protos/flyteidl/admin/workflow.proto#L16

katrogan · 2022-10-17T20:12:29Z

protos/flyteidl/core/workflow.proto

+}
+
+// The workflow id is already used with an identical sctructure
+message WorkflowErrorExistsIdenticalStructure {


I wonder if we need this message since we return a different gRPC status code (AlreadyExists) for this scenario

although perhaps it would be nice to keep this to include the existing workflow id 👍

katrogan

hey @jerempy thanks for opening this! can you try these instructions for generating proto code and let me know if that works for you?

jerempy · 2022-10-17T22:25:41Z

hey @jerempy thanks for opening this! can you try these instructions for generating proto code and let me know if that works for you?

Hey there, I did try that and had difficulties. It looks dated since there is no "download_tooling" in the makefile. I tried looking at different things in the make file and couldn't figure out what I was missing. Any other ideas?

katrogan · 2022-10-17T22:27:46Z

ah @jerempy the top-level Makefile should import the boilerplate one with the target:

flyteidl/boilerplate/flyte/golang_test_targets/Makefile

Line 7 in b24a555

.PHONY: download_tooling

jerempy · 2022-10-18T03:14:05Z

@katrogan thanks for all the input! I updated the type for the id - good spotting that!
Thanks for the direction on getting the compiler to work, but I just seem to not be having any luck. Maybe its cuz im on windows 10. I tried through my cmd prompt. I tried through wsl, and I tried to git bash.

When trying to run make command in wsl I would get this error:

Makefile:17: warning: overriding recipe for target 'generate'
boilerplate/flyte/golang_test_targets/Makefile:13: warning: ignoring old recipe for target 'generate'
Makefile:30: warning: overriding recipe for target 'test_unit'
boilerplate/flyte/golang_test_targets/Makefile:38: warning: ignoring old recipe for target 'test_unit'
make: boilerplate/flyte/golang_test_targets/download_tooling.sh: Command not found
make: *** [boilerplate/flyte/golang_test_targets/Makefile:9: download_tooling] Error 127

I tried running the .sh scripts directy and that didn't turn out so good. Wanted to delete 1200 files as a result 😨
So for now I just changed the types on the id and I think I'm a bit out of luck on this one. unfortunately. I might try again tomorrow

katrogan · 2022-10-18T19:02:54Z

hey @jerempy do you mind filing an issue dumping what you ran into? I'm sorry this process doesn't work out of the box :(

I can help you with the generation for now. But in the meantime, could I ask you to move the new proto message definitions to the admin workflow proto def: https://github.com/flyteorg/flyteidl/blob/master/protos/flyteidl/admin/workflow.proto rather than the core one you've defined them in now?

Signed-off-by: jerempy <[email protected]>

jerempy · 2022-10-18T20:39:28Z

@katrogan ok great. thanks much for your help! and good spottin on the location - I moved it. Thanks! 🙏

Signed-off-by: Katrina Rogan <[email protected]>

codecov · 2022-10-18T22:19:30Z

Codecov Report

Merging #331 (8a7a0df) into master (70aea35) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #331   +/-   ##
=======================================
  Coverage   75.57%   75.57%           
=======================================
  Files          18       18           
  Lines        1171     1171           
=======================================
  Hits          885      885           
  Misses        235      235           
  Partials       51       51

Flag	Coverage Δ
unittests	`75.16% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

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

katrogan · 2022-10-18T23:33:25Z

@EngHabu since you also reviewed the corresponding admin PR, mind reviewing this one too?

EngHabu

Thank you for taking on that work! Mind linking to an issue too?

EngHabu · 2022-10-19T00:00:54Z

protos/flyteidl/admin/workflow.proto

+}
+
+// When a CreateWorkflowRequest failes due to matching id
+message CreateWorkflowFailureReason {


Should we do the same for tasks?

What if we do that in a seperate issue/ pr series? I can make the issue for it as a follow up after finishing this one if you want.

That would be awesome!

EngHabu · 2022-10-19T00:01:24Z

protos/flyteidl/admin/workflow.proto

+
+// The workflow id is already used and the structure is different
+message WorkflowErrorExistsDifferentStructure {
+    core.Identifier id = 1;


Should we include the structure of the other workflow?

Current plan was to return the id of the workflow to the sdk client and then to get the workflow and as part of the exception handling to then compare the diff with the initial request

jerempy · 2022-10-19T00:38:15Z

@EngHabu thanks for reviewing! I updated the top comment to Include the issue. And thanks @katrogan for compiling for me!

jerempy · 2022-10-24T13:30:50Z

@EngHabu @katrogan if possible, think we could merge this today? After this one is done then I still need to re-do the flyeadmin PR and then the flytekit PR to get this issue resolved and I only got 7 days left until the end of the month 🙏

EngHabu · 2022-10-24T22:38:49Z

protos/flyteidl/admin/workflow.proto

+}
+
+// When a CreateWorkflowRequest failes due to matching id
+message CreateWorkflowFailureReason {


That would be awesome!

welcome · 2022-10-24T23:51:26Z

Congrats on merging your first pull request! 🎉

katrogan reviewed Oct 17, 2022

View reviewed changes

jerempy added 3 commits October 18, 2022 16:38

add createworkflow failure pb

a89933f

Signed-off-by: jerempy <[email protected]>

better id type

7b6f966

Signed-off-by: jerempy <[email protected]>

move proto to admin

1bdb2eb

Signed-off-by: jerempy <[email protected]>

jerempy mentioned this pull request Oct 18, 2022

[Housekeeping] Makefile commands not working on windows 10 flyteorg/flyte#2997

Open

2 tasks

gen

8a7a0df

Signed-off-by: Katrina Rogan <[email protected]>

EngHabu reviewed Oct 19, 2022

View reviewed changes

jerempy requested review from EngHabu and removed request for EngHabu October 21, 2022 11:50

EngHabu approved these changes Oct 24, 2022

View reviewed changes

katrogan approved these changes Oct 24, 2022

View reviewed changes

katrogan merged commit 6965b90 into flyteorg:master Oct 24, 2022

samhita-alla added the hacktoberfest-accepted label Oct 27, 2022

eapolinario pushed a commit that referenced this pull request Sep 8, 2023

Add createworkflow failure proto #minor (#331)

1cbb9a9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add createworkflow failure proto #minor #331

Add createworkflow failure proto #minor #331

jerempy commented Oct 17, 2022 •

edited

Loading

welcome bot commented Oct 17, 2022

jerempy commented Oct 17, 2022

katrogan Oct 17, 2022

katrogan Oct 17, 2022

katrogan Oct 17, 2022

katrogan left a comment

jerempy commented Oct 17, 2022

katrogan commented Oct 17, 2022

jerempy commented Oct 18, 2022

katrogan commented Oct 18, 2022 •

edited

Loading

jerempy commented Oct 18, 2022

codecov bot commented Oct 18, 2022

katrogan commented Oct 18, 2022

EngHabu left a comment

EngHabu Oct 19, 2022

jerempy Oct 19, 2022

EngHabu Oct 24, 2022

EngHabu Oct 19, 2022

jerempy Oct 19, 2022

jerempy commented Oct 19, 2022

jerempy commented Oct 24, 2022

EngHabu Oct 24, 2022

welcome bot commented Oct 24, 2022

Add createworkflow failure proto #minor #331

Add createworkflow failure proto #minor #331

Conversation

jerempy commented Oct 17, 2022 • edited Loading

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

welcome bot commented Oct 17, 2022

jerempy commented Oct 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katrogan left a comment

Choose a reason for hiding this comment

jerempy commented Oct 17, 2022

katrogan commented Oct 17, 2022

jerempy commented Oct 18, 2022

katrogan commented Oct 18, 2022 • edited Loading

jerempy commented Oct 18, 2022

codecov bot commented Oct 18, 2022

Codecov Report

katrogan commented Oct 18, 2022

EngHabu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerempy commented Oct 19, 2022

jerempy commented Oct 24, 2022

Choose a reason for hiding this comment

welcome bot commented Oct 24, 2022

jerempy commented Oct 17, 2022 •

edited

Loading

katrogan commented Oct 18, 2022 •

edited

Loading