-
Notifications
You must be signed in to change notification settings - Fork 36
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
Programmatically build workflow #190
Programmatically build workflow #190
Conversation
Signed-off-by: André R. de Miranda <andre@galgo.tech>
0cb6bb2
to
e3b52c5
Compare
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 improvement to the SDK! A few suggestions, if you don't mind!
model/builder/workflow.go
Outdated
return workflowBuilder, builder | ||
} | ||
|
||
type WorkflowBuilder struct { |
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.
What about an interface instead? Hence, we can model different builders if needed and have a clear API.
Also, can we have a WorkflowBuilder.BuildAsYaml(path)
? This way we can persist into a file programmatically. wdyt?
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 builder interface has been decoupled from the WorkflowBuilder
, so it is possible to have an ActionBuilder
It is possible to do this as follows:
type Builder[T, K any] struct {
builder Build[K]
}
func (b *Builder[T, K]) Build() K {
return b.builder.Build()
}
func (b *Builder[T, K]) BuildAsYaml() string {
return b.builder.BuildAsYaml()
}
type Build[T any] interface {
Build() T
BuildAsYaml() string
}
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.
Fair enough!
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.
Can you implement it in this PR? We may use our functions to generate the file.
model/builder/workflow.go
Outdated
return workflowBuilder, builder | ||
} | ||
|
||
type WorkflowBuilder struct { |
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.
Can you implement it in this PR? We may use our functions to generate the file.
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.
Very nice, just one thing, maybe improve the tests.
Added a small comment about it.
Other than that, just address the open Zanini's questions. :)
Thanks very much!
model/builder/workflow_test.go
Outdated
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestNewWorkflowBuilder(t *testing.T) { |
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.
for the test, can you please improve it?
E.g., use a complete workflow that we have in the examples, build it using the Builder, then parse it to yaml or json and compare the final result with the example we have used as base.
does it make sense?
@ribeiromiranda hi, do you still have plans to continue working on this PR? Thanks. |
@spolti I'm looking at creating a "builder-gen" as the "deepcopy-gen". But, I'm short on time. |
Nice, no problem, take your time and many thanks!!! |
Signed-off-by: André R. de Miranda <andre@galgo.tech>
Signed-off-by: André R. de Miranda <andre@galgo.tech>
Generated the file Some pending issues detected:
Generator used: |
Signed-off-by: André R. de Miranda <andre@galgo.tech>
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.
Pretty cool!
Signed-off-by: André R. de Miranda <andre@galgo.tech>
Signed-off-by: André R. de Miranda <andre@galgo.tech>
fc09bbb
to
2f0f3ef
Compare
Signed-off-by: André R. de Miranda <andre@galgo.tech>
@ribeiromiranda when you finish, can you let us know so we can do a final review/ |
@ricardozanini I think for a first version is ready for review. |
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.
LGTM
Closes #154 I missed the CI from my notifications, merging it. |
Proposal for programmatically build workflow definitions.
Example: