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

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

Merged
merged 19 commits into from
Sep 21, 2024

Conversation

eduardodbr
Copy link
Member

@eduardodbr eduardodbr commented Jul 19, 2024

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:

Verification

Current:

argo cron list

image

argo cron get

image

With this PR changes:

argo cron list

image

argo cron get

image

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

image

@eduardodbr 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 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 agilgur5 added this to the v3.6.0 milestone Jul 20, 2024
Copy link
Contributor

@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.

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

@eduardodbr eduardodbr marked this pull request as ready for review July 21, 2024 00:15
@eduardodbr eduardodbr marked this pull request as draft July 21, 2024 00:48
@eduardodbr eduardodbr marked this pull request as ready for review July 21, 2024 19:02
@eduardodbr
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

eduardodbr commented Jul 22, 2024

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

@agilgur5
Copy link
Contributor

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

@eduardodbr
Copy link
Member Author

eduardodbr commented Jul 22, 2024

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

@agilgur5
Copy link
Contributor

agilgur5 commented Jul 22, 2024

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

Comment on lines 159 to 161
{(w.spec.schedule ?? '') != ''
? w.spec.schedule
: w.spec.schedules.map(schedule => (
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@Joibel
Copy link
Member

Joibel commented Sep 20, 2024

@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
Copy link
Member Author

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

@agilgur5 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 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
Copy link
Contributor

@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.

some notes on in-line deprecation and versions

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

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
Copy link
Contributor

@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 this! Just a small comment below

ui/src/app/cron-workflows/cron-workflow-list.tsx Outdated Show resolved Hide resolved
@agilgur5 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
Copy link
Contributor

@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.

LGTM, thanks for iterating on this!

@agilgur5
Copy link
Contributor

Looks like there's some test failures in CI

Signed-off-by: eduardodbr <[email protected]>
@agilgur5 agilgur5 enabled auto-merge (squash) September 21, 2024 03:38
@agilgur5 agilgur5 merged commit 2dac126 into argoproj:main Sep 21, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants