Skip to content
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.

Sign up for GitHub

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

fix!: consolidate cronworkflow variables #13702

Merged
merged 4 commits into from
Oct 17, 2024
Merged

fix!: consolidate cronworkflow variables #13702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9147d8f
fix!: consolidate cronworkflow variables
Joibel Oct 3, 2024
3ec9cdf
chore: fix swaggerify breaking the comments
Joibel Oct 7, 2024
1c20373
docs: review comments
Joibel Oct 11, 2024
8c367ce
Merge branch 'main' into cron-variable-tidies
Joibel Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions api/jsonschema/schema.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions api/openapi-spec/swagger.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 6 additions & 6 deletions docs/cron-workflows.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ You can use `CronWorkflow.spec.workflowMetadata` to add `labels` and `annotation
| `startingDeadlineSeconds` | `0` | Seconds after [the last scheduled time](#crash-recovery) during which a missed `Workflow` will still be run. |
| `successfulJobsHistoryLimit` | `3` | Number of successful `Workflows` to persist |
| `failedJobsHistoryLimit` | `1` | Number of failed `Workflows` to persist |
| `stopStrategy` | `nil` | v3.6 and after: defines if the CronWorkflow should stop scheduling based on a condition |
| `stopStrategy.expression` | `nil` | v3.6 and after: defines if the CronWorkflow should stop scheduling based on an expression, which if present must evaluate to false for the workflow to be created |
Copy link

@agilgur5 agilgur5 Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| `stopStrategy.expression` | `nil` | v3.6 and after: defines if the CronWorkflow should stop scheduling based on an expression, which if present must evaluate to false for the workflow to be created |
| `stopStrategy.expression` | `nil` | v3.6 and after: defines if the CronWorkflow [should stop scheduling](#automatically-stopping-a-cronworkflow) based on an expression |

| `when` | None | v3.6 and after: An optional [expression](walk-through/conditionals.md) which will be evaluated on each cron schedule hit and the workflow will only run if it evaluates to `true` |

### Cron Schedule Syntax
Expand Down Expand Up @@ -150,30 +150,30 @@ The second will not run because of the `when` expression, which prevents this wo
> v3.6 and after

You can configure a `CronWorkflow` to automatically stop based on an [expression](variables.md#expression) with `stopStrategy.condition`.
You can use the [variables](variables.md#stopstrategy) `failed` and `succeeded`.
You can use the [variables](variables.md#cronworkflows) `cronworkflow.failed` and `cronworkflow.succeede2d`.

For example, if you want to stop scheduling new workflows after one success:

agilgur5 marked this conversation as resolved.
Show resolved Hide resolved
```yaml
stopStrategy:
condition: "succeeded >= 1"
expression: "cronworkflow.succeeded >= 1"
```

You can also stop scheduling new workflows after three failures with:

```yaml
stopStrategy:
condition: "failed >= 3"
expression: "cronworkflow.failed >= 3"
```

<!-- markdownlint-disable MD046 -- this is indented due to the admonition, not a code block -->
Joibel marked this conversation as resolved.
Show resolved Hide resolved
!!! Warning "Scheduling vs. Completions"
Depending on the time it takes to schedule and run a workflow, the number of completions can exceed the configured maximum.

For example, if you configure the `CronWorkflow` to schedule every minute (`* * * * *`) and stop after one success (`succeeded >= 1`).
For example, if you configure the `CronWorkflow` to schedule every minute (`* * * * *`) and stop after one success (`cronworkflow.succeeded >= 1`).
If the `Workflow` takes 90 seconds to run, the `CronWorkflow` will actually stop after two completions.
This is because when the stopping condition is achieved, there is _already_ another `Workflow` running.
For that reason, prefer conditions like `succeeded >= 1` over `succeeded == 1`.
For that reason, prefer conditions like `cronworkflow.succeeded >= 1` over `cronworkflow.succeeded == 1`.
<!-- markdownlint-enable MD046 -->

## Managing `CronWorkflow`
Expand Down
4 changes: 2 additions & 2 deletions docs/fields.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1994,12 +1994,12 @@ SynchronizationStatus stores the status of semaphore and mutex.

## StopStrategy

v3.6 and after: StopStrategy defines if the CronWorkflow should stop scheduling based on a condition
StopStrategy defines if the CronWorkflow should stop scheduling based on an expression. v3.6 and after

### Fields
| Field Name | Field Type | Description |
|:----------:|:----------:|---------------|
|`condition`|`string`|v3.6 and after: Condition is an expression that stops scheduling workflows when true. Use the variables `failed` or `succeeded` to access the number of failed or successful child workflows.|
|`expression`|`string`|v3.6 and after: Expression is an expression that stops scheduling workflows when true. Use the variables `cronworkflow`.`failed` or `cronworkflow`.`succeeded` to access the number of failed or successful child workflows.|

## Artifact

Expand Down
13 changes: 2 additions & 11 deletions docs/variables.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,8 @@ Only available for `successCondition`
| `cronworkflow.annotations.<NAME>` | CronWorkflow annotations (`string`) |
| `cronworkflow.annotations.json` | CronWorkflow annotations as a JSON string (`string`) |
| `cronworkflow.lastScheduledTime` | The time since this workflow was last scheduled, value is nil on first run (`*time.Time`) |
| `cronworkflow.failed` | Counts how many times child workflows failed |
| `cronworkflow.succeeded` | Counts how many times child workflows succeeded |

### `RetryStrategy`

Expand Down Expand Up @@ -290,17 +292,6 @@ For `Template`-level metrics:
| `workflow.status` | Workflow status. One of: `Succeeded`, `Failed`, `Error` |
| `workflow.failures` | A list of JSON objects containing information about nodes that failed or errored during execution. Available fields: `displayName`, `message`, `templateName`, `phase`, `podName`, and `finishedAt`. |

### `stopStrategy`

> v3.6 and after

When using the `condition` field within the [`stopStrategy` of a `CronWorkflow`](cron-workflows.md#automatically-stopping-a-cronworkflow), special variables are available.

| Variable | Description|
|----------|------------|
| `failed` | Counts how many times child workflows failed |
| `succeeded` | Counts how many times child workflows succeeded |

### Knowing where you are

The idea with creating a `WorkflowTemplate` is that they are reusable bits of code you will use in many actual Workflows. Sometimes it is useful to know which workflow you are part of.
Expand Down
4 changes: 2 additions & 2 deletions manifests/base/crds/full/argoproj.io_cronworkflows.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions pkg/apis/workflow/v1alpha1/cron_workflow_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,11 @@ type CronWorkflowSpec struct {
When string `json:"when,omitempty" protobuf:"bytes,12,opt,name=when"`
}

// v3.6 and after: StopStrategy defines if the CronWorkflow should stop scheduling based on a condition
// StopStrategy defines if the CronWorkflow should stop scheduling based on an expression. v3.6 and after
Copy link

@agilgur5 agilgur5 Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version annotation being consistently the first thing is intentional so that it is the first thing you see. This change makes this inconsistent with every other version annotation in the codebase, like lines 64-68 above, as well as harder to notice (and we already have trouble with users not noticing versions and so should not increase that).

I believe you made this change for GoDoc purposes, which does raise a conundrum since that apparently requires it to start the same way? If it's just a convention, the consistency and visibility should supersede that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did that to make it GoDoc compliant.

Copy link

@agilgur5 agilgur5 Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does not address my comment (I mentioned that myself in my comment) and, as I wrote above, is inconsistent with the entirety of rest of the code and docs (where this exact comment is user-facing) on version annotation placement

type StopStrategy struct {
// v3.6 and after: Condition is an expression that stops scheduling workflows when true. Use the
// variables `failed` or `succeeded` to access the number of failed or successful child workflows.
Condition string `json:"condition" protobuf:"bytes,1,opt,name=condition"`
// v3.6 and after: Expression is an expression that stops scheduling workflows when true. Use the variables
// `cronworkflow`.`failed` or `cronworkflow`.`succeeded` to access the number of failed or successful child workflows.
Expression string `json:"expression" protobuf:"bytes,1,opt,name=expression"`
}

// CronWorkflowStatus is the status of a CronWorkflow
Expand Down
Loading
Loading