-
Notifications
You must be signed in to change notification settings - Fork 220
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
Support disabling run following for workflow results #791
Conversation
// workflow may not use the literal run ID but instead follow to later runs | ||
// if the workflow returned a ContinueAsNewError, has a later cron, or is | ||
// retried on failure. | ||
DisableFollowingRuns bool |
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 I fail if this was set to true but a run ID was not provided? Is there a use case for not providing a run ID but still not following to the next run?
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 don't in TS. I can't think of a reason not to allow it, saves looking up the last run ID for a workflow ID.
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 we change this setting name to FollowRuns
and still default to true
? It's easier to follow.
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 we change this setting name to FollowRuns and still default to true? It's easier to follow.
I am afraid not in Go. You can't default fields, their default is always the zero value which for booleans is false. This is why for all Go structs, you specifically build the field where false is the reasonable default.
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.
Yeah, I get it. I thought that we could check if the options struct is null and default to true but we want empty struct to also default to true.
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 options struct is never null inside the outer options (it's not a pointer). And even if it could be nullable, we want to support people creating it and just setting the fields they care about instead of trying to differentiate between unset and explicit false.
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.
Cool, I agree this is the best approach
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.
Took a brief look and left a comment inline for your consideration.
What was changed
WorkflowRun.GetWithOptions
that supports an option to disable following runsWhy?
Currently we always follow to the last run in the "chain". Some people don't want this, especially for crno (see #789).
Checklist