fix(cron): various follow-ups for multiple schedules #13369

eduardodbr · 2024-07-19T21:55:27Z

Motivation

Currently argo cron list and argo cron get are showing the schedules with timezone, when available. This PR removes it as it doesn't bring any value because it can be seen in the timezone field.

Modifications

The schedule string can now be requested with and without the timezone.
With timezone, it is used for the annotation last-used-schedule, so changing the timezone while keeping the schedule will be seen as a new schedule.
Without timezone, it is used in argo cron CLI.

Also:

update every instance of schedule to be schedules as suggested by @agilgur5 feat(cron): allow multiple schedules #12616 (comment)
update fields in CronWorkflows table for multiple schedules per feat(cron): allow multiple schedules #12616 (review)
add in-line version references and deprecation per feat(cron): allow multiple schedules #12616 (comment)
add example

Verification

Current:

argo cron list

argo cron get

With this PR changes:

argo cron list

argo cron get

The CronWorkflow label last-used-schedule still uses the schedule with timezone

Signed-off-by: eduardodbr <[email protected]>

agilgur5

It might make sense to split out the schedule -> schedules change #12616 (comment) into its own PR. That and the v3.6 and after: and other #12616 review follow-ups can be merged pretty much immediately while you're still working out the test failures etc here. And the v3.6 and after: portion is particularly useful for people who are looking at the latest docs.

Also there was one miss from my previous review #12616 (review), the CronWorkflow docs page's fields table

Signed-off-by: eduardodbr <[email protected]>

eduardodbr · 2024-07-21T19:06:57Z

whille implementing this, I also found that schedule was still marked as required in the open api spec/json schema/CRD spec so I removed it. Do you think it should have a dedicated PR or is it ok to have it here? @agilgur5

agilgur5 · 2024-07-21T19:48:38Z

Is the CLI fix only relevant to 3.6? Or does it also impact 3.5/3.4?

If these are all just follow-up fixes to the multiple schedules PR, we can leave it all as one I suppose -- just would rename the PR.

If the CLI fix is relevant to 3.5/3.4, then I think we should split the rest out, so that it can be backported

eduardodbr · 2024-07-22T18:00:55Z

this only applies to 3.6 as it was introduced in #12616

agilgur5 · 2024-07-22T18:05:21Z

I know schedules was, but I wasn't sure if this TZ duplication happened in earlier versions with single schedule as well

eduardodbr · 2024-07-22T18:08:39Z

the timezone in argo cron list and argo cron get was also introduced in that PR

agilgur5 · 2024-07-22T18:29:35Z

Ok gotcha, wanted to make sure since I hadn't reviewed that whole PR. Then let's just fix-up the title and description to include all follow-ups as one

Also btw, no need to quote reply if you're replying to the whole thing in order. Quoting is useful for replying to parts at a time or out of order comments. In this case they read fluidly down and so quoting actually reads duplicatively (and if everyone does it, causes those massive quote of quote of quote of quotes). Forum kiddie me of the past is happy to have learned that lol

ui/src/app/cron-workflows/cron-workflow-list.tsx

agilgur5 · 2024-07-25T01:08:43Z

ui/src/app/cron-workflows/cron-workflow-list.tsx

+                                            {(w.spec.schedule ?? '') != ''
                                                ? w.spec.schedule
                                                : w.spec.schedules.map(schedule => (


Suggested change

{(w.spec.schedule ?? '') != ''

? w.spec.schedule

: w.spec.schedules.map(schedule => (

{w.spec.schedule || w.spec.schedules.map(schedule => (

this specific one can be further simplified

although tbh normalizing schedule to a single element array of schedules would be simpler to do everywhere

Joibel · 2024-09-20T10:25:47Z

@eduardodbr, any chance that you'd have a chance to fix/update this so it can go into 3.6?

We're currently missing the documentation on multiple schedules, which is in this PR.

eduardodbr · 2024-09-20T10:53:54Z

Hi @Joibel will try to finish this ASAP, sorry for the delay

agilgur5

some notes on in-line deprecation and versions

I also updated the PR title and description per my previous comment

docs/cron-workflows.md

pkg/apis/workflow/v1alpha1/cron_workflow_types.go

examples/cron-workflow-multiple-schedules.yaml

Signed-off-by: eduardodbr <[email protected]>

Co-authored-by: Anton Gilgur <4970083 [email protected]> Signed-off-by: Eduardo Rodrigues <[email protected]>

Signed-off-by: eduardodbr <[email protected]>

agilgur5

Thanks for updating this! Just a small comment below

ui/src/app/cron-workflows/cron-workflow-list.tsx

Signed-off-by: eduardodbr <[email protected]>

ui/src/app/cron-workflows/cron-workflow-list.tsx

Signed-off-by: eduardodbr <[email protected]>

agilgur5

LGTM, thanks for iterating on this!

agilgur5 · 2024-09-21T01:35:00Z

Looks like there's some test failures in CI

Signed-off-by: eduardodbr <[email protected]>

eduardodbr added 3 commits July 19, 2024 21:55

fix(cli): argo cron list and get to show schedules without timezone

760e4f4

Signed-off-by: eduardodbr <[email protected]>

fix: allow cron operator to keep using the schedule with timezone

b09a29c

Signed-off-by: eduardodbr <[email protected]>

fix: last-used-schedule annotation should use the timezone

181d032

Signed-off-by: eduardodbr <[email protected]>

eduardodbr changed the title ~~Argo cli get list schedules~~ fix(cli): argo cron list and get to show schedules without timezone Jul 19, 2024

agilgur5 changed the title ~~fix(cli): argo cron list and get to show schedules without timezone~~ fix(cli): argo cron list and get to show schedules without timezone Jul 19, 2024

agilgur5 added area/ui area/cli The `argo` CLI area/cron-workflows labels Jul 19, 2024

agilgur5 mentioned this pull request Jul 20, 2024

feat(cron): allow multiple schedules #12616

Merged

agilgur5 added this to the v3.6.0 milestone Jul 20, 2024

agilgur5 reviewed Jul 20, 2024

View reviewed changes

eduardodbr added 3 commits July 20, 2024 21:53

fix: cron GetScheduleString unit tests

19de0bc

Signed-off-by: eduardodbr <[email protected]>

fix: merge conflicts

70898fc

Signed-off-by: eduardodbr <[email protected]>

fix: e2e tests

26828d4

Signed-off-by: eduardodbr <[email protected]>

eduardodbr marked this pull request as ready for review July 21, 2024 00:15

eduardodbr requested review from terrytangyuan and sarabala1979 as code owners July 21, 2024 00:15

docs: add schedules to cron workflow docs

982ecde

Signed-off-by: eduardodbr <[email protected]>

eduardodbr marked this pull request as draft July 21, 2024 00:48

fix: make schedule optional in cron workflows

7d5aae5

Signed-off-by: eduardodbr <[email protected]>

eduardodbr marked this pull request as ready for review July 21, 2024 19:02

agilgur5 reviewed Jul 25, 2024

View reviewed changes

ui/src/app/cron-workflows/cron-workflow-list.tsx Outdated Show resolved Hide resolved

agilgur5 reviewed Jul 25, 2024

View reviewed changes

agilgur5 changed the title ~~fix(cli): argo cron list and get to show schedules without timezone~~ fix: docs, UI, CLI, and test follow-ups for multiple schedules Sep 20, 2024

agilgur5 changed the title ~~fix: docs, UI, CLI, and test follow-ups for multiple schedules~~ fix(cron): docs, UI, CLI, and test follow-ups for multiple schedules Sep 20, 2024

agilgur5 reviewed Sep 20, 2024

View reviewed changes

docs/cron-workflows.md Outdated Show resolved Hide resolved

pkg/apis/workflow/v1alpha1/cron_workflow_types.go Outdated Show resolved Hide resolved

examples/cron-workflow-multiple-schedules.yaml Outdated Show resolved Hide resolved

eduardodbr and others added 7 commits September 21, 2024 00:49

chore: merge master

42f6ab6

Signed-off-by: eduardodbr <[email protected]>

fix: next run on multiple schedules

331d8f7

Signed-off-by: eduardodbr <[email protected]>

fix: use truthy on cron workflow schedule list

09ff0c3

Signed-off-by: eduardodbr <[email protected]>

docs: Update docs/cron-workflows.md to set schedule as deprecated

5f0b978

Co-authored-by: Anton Gilgur <4970083 [email protected]> Signed-off-by: Eduardo Rodrigues <[email protected]>

docs: set schedule as deprecated in cron_workflow_types

095575e

Co-authored-by: Anton Gilgur <4970083 [email protected]> Signed-off-by: Eduardo Rodrigues <[email protected]>

docs: fix examples lint in cron-workflow-multiple-schedules.yaml

42af73f

Co-authored-by: Anton Gilgur <4970083 [email protected]> Signed-off-by: Eduardo Rodrigues <[email protected]>

fix: pre-commit

ba455b6

Signed-off-by: eduardodbr <[email protected]>

agilgur5 reviewed Sep 21, 2024

View reviewed changes

ui/src/app/cron-workflows/cron-workflow-list.tsx Outdated Show resolved Hide resolved

agilgur5 changed the title ~~fix(cron): docs, UI, CLI, and test follow-ups for multiple schedules~~ fix(cron): various follow-ups for multiple schedules Sep 21, 2024

chore: simple code on getSpecNextScheduledTime

a8c38cb

Signed-off-by: eduardodbr <[email protected]>

agilgur5 reviewed Sep 21, 2024

View reviewed changes

ui/src/app/cron-workflows/cron-workflow-list.tsx Outdated Show resolved Hide resolved

eduardodbr added 2 commits September 21, 2024 02:22

fix: codegen

b7db363

Signed-off-by: eduardodbr <[email protected]>

fix: remove unnecessary check

b72f92f

Signed-off-by: eduardodbr <[email protected]>

agilgur5 approved these changes Sep 21, 2024

View reviewed changes

fix: test TestCronCommands

d0cead7

Signed-off-by: eduardodbr <[email protected]>

agilgur5 enabled auto-merge (squash) September 21, 2024 03:38

agilgur5 merged commit 2dac126 into argoproj:main Sep 21, 2024
27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cron): various follow-ups for multiple schedules #13369

fix(cron): various follow-ups for multiple schedules #13369

eduardodbr commented Jul 19, 2024 •

edited by agilgur5

Loading

agilgur5 left a comment •

edited

Loading

eduardodbr commented Jul 21, 2024

agilgur5 commented Jul 21, 2024

eduardodbr commented Jul 22, 2024 •

edited by agilgur5

Loading

agilgur5 commented Jul 22, 2024

eduardodbr commented Jul 22, 2024 •

edited by agilgur5

Loading

agilgur5 commented Jul 22, 2024 •

edited

Loading

agilgur5 Jul 25, 2024

agilgur5 Jul 25, 2024

Joibel commented Sep 20, 2024

eduardodbr commented Sep 20, 2024

agilgur5 left a comment •

edited

Loading

agilgur5 left a comment

agilgur5 left a comment

agilgur5 commented Sep 21, 2024

fix(cron): various follow-ups for multiple schedules #13369

fix(cron): various follow-ups for multiple schedules #13369

Conversation

eduardodbr commented Jul 19, 2024 • edited by agilgur5 Loading

Motivation

Modifications

Verification

agilgur5 left a comment • edited Loading

Choose a reason for hiding this comment

eduardodbr commented Jul 21, 2024

agilgur5 commented Jul 21, 2024

eduardodbr commented Jul 22, 2024 • edited by agilgur5 Loading

agilgur5 commented Jul 22, 2024

eduardodbr commented Jul 22, 2024 • edited by agilgur5 Loading

agilgur5 commented Jul 22, 2024 • edited Loading

agilgur5 Jul 25, 2024

Choose a reason for hiding this comment

agilgur5 Jul 25, 2024

Choose a reason for hiding this comment

Joibel commented Sep 20, 2024

eduardodbr commented Sep 20, 2024

agilgur5 left a comment • edited Loading

Choose a reason for hiding this comment

agilgur5 left a comment

Choose a reason for hiding this comment

agilgur5 left a comment

Choose a reason for hiding this comment

agilgur5 commented Sep 21, 2024

eduardodbr commented Jul 19, 2024 •

edited by agilgur5

Loading

agilgur5 left a comment •

edited

Loading

eduardodbr commented Jul 22, 2024 •

edited by agilgur5

Loading

eduardodbr commented Jul 22, 2024 •

edited by agilgur5

Loading

agilgur5 commented Jul 22, 2024 •

edited

Loading

agilgur5 left a comment •

edited

Loading