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

[JENKINS-9283] Timezone Support for Scheduling #1672

Merged
merged 1 commit into from
May 21, 2015

Conversation

0bp
Copy link
Contributor

@0bp 0bp commented Apr 25, 2015

This will allow to define a timezone in the scheduling text input field like in some cron implementations. If there's for example TZ=Australia/Sydney defined in the first line, each cron definition in that field will trigger the job based on that timezone. The timezone defined in the job does not affect the date/times for the execution history. Those times are still in the systems timezone.

Supported timezones are those from TimeZone.getAvailableIDs()

@@ -71,6 +75,16 @@ public String checkSanity() {
return null;
}

public static String getValidTimezone(String timezone) {
Copy link
Member

Choose a reason for hiding this comment

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

@CheckForNull is a must in such methods. Javadoc would be also useful

@0bp 0bp changed the title [JENKINS-9283] Timezone Support for Scheduling [WIP] [JENKINS-9283] Timezone Support for Scheduling Apr 26, 2015
@0bp 0bp changed the title [WIP] [JENKINS-9283] Timezone Support for Scheduling [JENKINS-9283] Timezone Support for Scheduling Apr 27, 2015
@oleg-nenashev
Copy link
Member

LGTM

@oleg-nenashev
Copy link
Member

@0bp please squash commits into a single one. After that I'll be able to merge the change

@0bp
Copy link
Contributor Author

0bp commented May 19, 2015

@oleg-nenashev Done, thanks!

oleg-nenashev added a commit that referenced this pull request May 21, 2015
@oleg-nenashev oleg-nenashev merged commit 73e151f into jenkinsci:master May 21, 2015
oleg-nenashev added a commit that referenced this pull request May 21, 2015
@oleg-nenashev
Copy link
Member

Merged, thanks!

@orrc
Copy link
Member

orrc commented May 26, 2015

It looks like documentation for this is missing, and JIRA hasn't been updated.

@oleg-nenashev
Copy link
Member

@orrc
Sorry, I've missed the documentation during the PR review. I'll add it as a second PR

}
}
return null;
}
Copy link
Member

Choose a reason for hiding this comment

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

Probably it's not a big deal, but I'd note that this check rules out using valid Java TimeZone values like GMT-8 as the TZ value.

@oleg-nenashev
Copy link
Member

@orrc
Thanks a lot for the comments. Since the change has been already merged, I propose the following approach:

  • I'm patching the help file for TimerTrigger with the description of the current format
  • I or @0bp will also update unit tests
  • Format validation
  • We can create an extra issue to improve the formatting later (inline TZ specification, etc.)

Would it work for you?

@orrc
Copy link
Member

orrc commented May 26, 2015

According to the code, TZ is being specified in the first line, which does not contain any other data.

Yeah, thanks. I realised my mistake and posted the same comment at the same time as you :)

I propose the following approach

Sounds good. I think clear documentation is important for this — I just found this feature via the 1.615 changelog and wondered how it worked.

I think form validation is also really important here, as it's easy to mistype a timezone ID, and then there's no way to find out that you made a mistake, without configuring a logger on CronTabList.

@oleg-nenashev
Copy link
Member

@orrc
Added the form validation to the checklist above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants