-
Notifications
You must be signed in to change notification settings - Fork 35
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
sdk-go proposal for DSL v1.0.0-alpha2 #204
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: André R. de Miranda <[email protected]>
Signed-off-by: André R. de Miranda <[email protected]>
Signed-off-by: André R. de Miranda <[email protected]>
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.
Many thanks! I have a few observations before we merge this:
- We should migrate the testdata to the new DSL and reuse to validate the new parser
- The Kubernetes deepcopy/codegen should be re-added
I really liked the Lookup
approach to fetch nodes from the workflow.
package dsl | ||
|
||
const DSLSpec = ` | ||
$id: https://serverlessworkflow.io/schemas/1.0.0-alpha1/workflow.yaml |
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.
We can upgrade :D
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.
Maybe we can think of a way to update this automatically via CI. Once you open the issues to follow up on this PR, can you open this one? I can take care of it.
@@ -37,7 +36,7 @@ import ( | |||
|
|||
// ServerlessWorkflowSpec defines a base API for integration test with operator-sdk | |||
type ServerlessWorkflowSpec struct { | |||
model.Workflow `json:",inline"` | |||
// model.Workflow `json:",inline"` |
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.
Leftovers?
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.
It was kept to adapt to the Kubernetes Controller in the future.
Will this Kubernetes folder be kept?
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.
Yes, the SDK must be compatible with Kubernetes objects.
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.
The kubernetes controller-gen requires struct
to work.
I tested the tool (https://github.com/omissis/go-jsonschema) to generate the struct
, but it gives an error with interface
when generating the code with deepcopy.
Using go-jsonschema or another tool seems like a solution.
Signed-off-by: André R. de Miranda <[email protected]>
Signed-off-by: André R. de Miranda <[email protected]>
Signed-off-by: André R. de Miranda <[email protected]>
@@ -0,0 +1,2246 @@ | |||
{ | |||
"$id": "https://serverlessworkflow.io/schemas/1.0.0/workflow.yaml", |
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.
This is a recurrent tech debt we have with other SDKs as well. Instead of reading from local, can we read from a URL? We have this: https://serverlessworkflow.io/schemas/1.0.0-alpha2/workflow.json
I'll automate publishing these schemas.
Discussion in the issue 203