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
Add warning about using unsupported CRON_TZ #106455
Conversation
/triage accepted |
/retest |
/hold |
/lgtm |
a98b0fb
to
8bbbc3c
Compare
return cj, nil, nil | ||
} | ||
|
||
if strings.Contains(cj.Spec.Schedule, "CRON_TZ") { | ||
jm.recorder.Eventf(cj, corev1.EventTypeWarning, "UnsupportedSchedule", "CRON_TZ used in schedule %q", cj.Spec.Schedule) |
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 this explain more about why this is bad? Or link to an explanation?
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.
@liggitt
Is it OK to add this link?
https://pkg.go.dev/github.com/robfig/cron/v3#hdr-Time_zones
2f9514d
to
f702305
Compare
if strings.Contains(cj.Spec.Schedule, "CRON_TZ") { | ||
jm.recorder.Eventf(cj, corev1.EventTypeWarning, "UnsupportedSchedule", "CRON_TZ used in schedule %q is not officially supported, see https://kubernetes.io/docs/concepts/workloads/controllers/cron-jobs/ for more details", cj.Spec.Schedule) | ||
} | ||
if strings.Contains(cj.Spec.Schedule, "TZ") { |
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.
the event will hit twice if we have CRON_TZ
. Same in strategy.go
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.
True, lemme fix that.
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.
Updated.
CRON_TZ variable slipped in during upgrading github.com/robfig/cron library. It allows setting a time zone which is a long requested feature but one that is not officially supported. This adds warning event since users should not rely on unsupported features.
f702305
to
d051884
Compare
/hold cancel |
/milestone v1.23 |
For the release note, can we say which component emits the Actually, the Warning: header is probably more prominent than the event, so maybe focus on that? |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: atiratree, cndoit18, liggitt, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
It's a bit sad (from user perspective) to see much effort to unsupport timezone vs support timezone. |
There'll be even more work on the KEP to do this properly, I suspect. |
Updated
It's not that we don't want to support timezones, but we can't let users rely on implementation detail which if we decided to switch the library would be gone. As Tim mentioned, we're hoping to add timezone support in the next release as alpha. |
For those who are not in the loop, this is relevant to #47202 |
Also a reference to the documentation fix which also clarifies this: kubernetes/website#30509 |
…455-upstream-release-1.22 Automated cherry pick of #106455: Add warning about using unsupported CRON_TZ
What type of PR is this?
/kind bug
/kind cleanup
/kind documentation
/kind regression
What this PR does / why we need it:
CRON_TZ variable slipped in during upgrading github.com/robfig/cron in library. It allows setting a time zone which is a long requested feature but one that is not officially supported. This adds warning event since users should not rely on unsupported features.
Special notes for your reviewer:
/assign @atiratree
Does this PR introduce a user-facing change?