-
Notifications
You must be signed in to change notification settings - Fork 18
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
Eng 2733 implement v2 list workflow route #1213
Eng 2733 implement v2 list workflow route #1213
Conversation
@@ -17,6 +17,9 @@ | |||
|
|||
|
|||
class TestBackend: | |||
V2_GET_WORKFLOWS_TEMPLATE = "/api/v2/workflows" | |||
|
|||
GET_WORKFLOWS_TEMPLATE = "/api/workflows" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep the v1 route around if we are moving over to v2?
Who else is using this route? Is it just our SDK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using it for testing the endpoint (assert len(v1_resp) == len(v2_resp)
). I think in the future we would want to remove it but I think while it is not deprecated it may be nice using it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually on second thought I've decided to remove the dependency on V1.
Code here looks good on my end. I'll defer to @likawind if there are any backend concerns that he may have. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Try to rebase on main as we moved around some codes in UI. Also, to follow naming of other files let's use <routeName><Method>
to name files and objects, we should use workflows_get
and WorkflowsGet
in various places
…3-implement-v2-list-workflow-route
List_StorageMigration, which is what I was referencing, doesn't seem to follow the naming convention of so we may want to make a task for that. I've updated the naming. I am a little confused when to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! WorkflowsGet
is the right convention. Workflows
is the resource name and Get
is the http method name. Let's try to run run_linters.py
and see if it helps resolves python formattings
Describe your changes and why you are making these changes
Implement the list workflow route
Related issue number (if any)
ENG 2733
Loom demo (if any)
Passed the test added to
integration_tests/backend/test_reads.py
Sample output:
Checklist before requesting a review
python3 scripts/run_linters.py -h
for usage).run_integration_test
: Runs integration testsskip_integration_test
: Skips integration tests (Should be used when changes are ONLY documentation/UI)