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.

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

docs: add CronWorkflow stopStrategy docs #12696

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

eduardodbr
Copy link
Member

Docs for stopStrategy implemented in #12305 to be released on v3.6

Signed-off-by: eduardodbr <eduardodbr@hotmail.com>
Signed-off-by: eduardodbr <eduardodbr@hotmail.com>
@eduardodbr eduardodbr marked this pull request as ready for review February 24, 2024 18:16
@agilgur5 agilgur5 self-assigned this Feb 24, 2024
@agilgur5 agilgur5 added area/docs Incorrect, missing, or mistakes in docs area/cron-workflows labels Feb 24, 2024
docs/cron-workflows.md Outdated Show resolved Hide resolved
Signed-off-by: eduardodbr <eduardodbr@hotmail.com>
Signed-off-by: eduardodbr <eduardodbr@hotmail.com>
Copy link
Member

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Thanks for updating based off my comments in #12305 (review) !

There's a few more modifications on top of that needed.

Otherwise I haven't had a chance to go through the docs section itself yet. From a glance, it will need some simplification and active voice changes. Also the expression docs should be linked here (see #12617 for examples)

docs/cron-workflows.md Outdated Show resolved Hide resolved
docs/fields.md Outdated Show resolved Hide resolved
Signed-off-by: eduardodbr <eduardodbr@hotmail.com>
Signed-off-by: Eduardo Rodrigues <eduardodbr@hotmail.com>
Signed-off-by: eduardodbr <eduardodbr@hotmail.com>
Signed-off-by: eduardodbr <eduardodbr@hotmail.com>
Signed-off-by: eduardodbr <eduardodbr@hotmail.com>
Copy link
Member

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Couple of simplifications and clarifications in-line below

@@ -103,6 +104,33 @@ For example, with timezone set at `America/Los_Angeles`, we have daylight saving
| | 2 | 2020-11-02 02:01:00 -0800 PST |
| | 3 | 2020-11-03 02:01:00 -0800 PST |

### Stopping a `CronWorkflow`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### Stopping a `CronWorkflow`
### Automatically Stopping a `CronWorkflow`

Manually stopping/suspending can already be done (e.g. argo cron suspsend), so this should differentiate.

Copy link
Member

Choose a reason for hiding this comment

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

Also I'm wondering if stopStrategy should entirely be renamed to suspendStrategy to be consistent? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

The cronworkflow is not set as "suspended", it is just removed from the queue and cannot become active again, so I think it is best to be different.

docs/cron-workflows.md Outdated Show resolved Hide resolved
docs/cron-workflows.md Outdated Show resolved Hide resolved
docs/cron-workflows.md Outdated Show resolved Hide resolved
pkg/apis/workflow/v1alpha1/cron_workflow_types.go Outdated Show resolved Hide resolved
Signed-off-by: eduardodbr <eduardodbr@hotmail.com>
@eduardodbr
Copy link
Member Author

Couple of simplifications and clarifications in-line below

The suggestions were pushed, thanks for the review!

@agilgur5 agilgur5 added this to the v3.6.0 milestone Apr 12, 2024
@eduardodbr
Copy link
Member Author

@agilgur5 do you think this is good to merge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cron-workflows area/docs Incorrect, missing, or mistakes in docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants