-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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(CLI-alpha): support backfill for cron workflow. Part of #2706 #13999
base: main
Are you sure you want to change the base?
Conversation
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 wonder if we should have a way to label this as something like a alpha feature,
so that we don't need to maintain backward compatibility incase there's a significant change later on
Namespace: Namespace, | ||
LabelRequirements: parse, | ||
}) | ||
s.CheckError(err) |
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 check backfill workflow actually exist, incase this returns empty list
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 referred to the previous implementation of cleaning/test label. If there is no error here, it doesn’t matter even if an empty list is returned. If the range of the subsequent loop is empty, delete will not be executed.
cmd/argo/commands/cron/backfill.go
Outdated
var scheList []string | ||
wf := common.ConvertCronWorkflowToWorkflow(cronWF) | ||
paramArg := `{{inputs.parameters.backfillscheduletime}}` | ||
wf.GenerateName = cronWF.Name + "-backfill-" + strings.ToLower(cliOps.name) + "-" |
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 may want to truncate this in case this gets too long
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.
Add a function GenerateBackfillWorkflowPrefix
docs/cli/argo_cron_backfill.md
Outdated
### Options | ||
|
||
``` | ||
--argname string Schedule time argument name for workflow (default "cronScheduleTime") |
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.
wondering if we really need this, it's only for parent workflow, right?
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,This is useful when the sub-workflow needs to accept time parameters, such as pulling data based on time parameters.
cmd/argo/commands/cron/backfill.go
Outdated
` | ||
|
||
func CreateMonitorWf(ctx context.Context, wf, namespace, cronWFName string, scheTime []string, wfClient workflow.WorkflowServiceClient, cliOps backfillOpts) error { | ||
const maxWfCount = 1000 |
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.
need to expose this as an argument?
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.
Set it as a parameter
Yes, it would be better if there could be an alpha feature label. I have now added an alpha field in the pr title. |
Perhaps let's add it to the CLI Short/Long description |
Co-authored-by: Saravanan Balasubramanian <[email protected]> Co-authored-by: shuangkun <[email protected]> Signed-off-by: shuangkun <[email protected]>
Signed-off-by: shuangkun <[email protected]>
Signed-off-by: shuangkun <[email protected]>
Signed-off-by: shuangkun <[email protected]>
Signed-off-by: shuangkun <[email protected]>
Signed-off-by: shuangkun <[email protected]>
Signed-off-by: shuangkun <[email protected]>
Signed-off-by: shuangkun <[email protected]>
Signed-off-by: shuangkun <[email protected]>
Signed-off-by: shuangkun <[email protected]>
Signed-off-by: shuangkun <[email protected]>
Signed-off-by: shuangkun <[email protected]>
Signed-off-by: shuangkun <[email protected]>
Co-authored-by: Tianchu Zhao <[email protected]> Signed-off-by: shuangkun tian <[email protected]>
Co-authored-by: Tianchu Zhao <[email protected]> Signed-off-by: shuangkun tian <[email protected]>
Signed-off-by: shuangkun <[email protected]>
Signed-off-by: shuangkun <[email protected]>
Signed-off-by: shuangkun <[email protected]>
Signed-off-by: shuangkun <[email protected]>
Signed-off-by: shuangkun <[email protected]>
Added alpha to CLI short description |
Signed-off-by: shuangkun <[email protected]>
Base #4212
Airflow is easy to backfill. Simplify the use of backfill on Argo.
Support:
Part of #2706
Motivation
Modifications
Verification