-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
feat(controller): add enum support in parameters (fixes #4192) #4314
feat(controller): add enum support in parameters (fixes #4192) #4314
Conversation
@alexec The
I haven't make changes in first 3. |
@alexec Have fixed issues, please let me know if any changes are required |
|
Thanks @alexec , the codegen issues are fixed and build is green, |
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.
Code changes look good. Need @jessesuen to approve photo change. Would be good to support this in the UI at some point.
@alexec I'll raise a UI PR after this gets merged. I actually have changes ready |
WaitForWorkflow(). | ||
Then(). | ||
ExpectWorkflow(func(t *testing.T, metadata *v1.ObjectMeta, status *v1alpha1.WorkflowStatus) { | ||
assert.Equal(t, status.Phase, v1alpha1.NodeSucceeded) |
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.
don't we want to test failure here?
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.
No, workflow-template-with-enum-values.yaml
contains a valid workflow
It should not result in an error
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.
@alexec Should I add a separate failing test
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 think if you can cover it with unit tests - we do not need an e2e test.
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.
approval of type changes
@alexec Thanks for merging. Will let you know when the UI PR is ready |
Enhancement proposal - #4192
UI PR coming up next
Checklist:
"fix(controller): Updates such and such. Fixes #1234"
.