Allow specification of timezones in Periodic Jobs #2321
Conversation
nomad/structs/structs.go
Outdated
| // Load the location | ||
| l, err := time.LoadLocation(p.TimeZone) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("Bad location %q: %v", p.TimeZone, err)) |
There was a problem hiding this comment.
Why panic? This panic should at least be mentioned in a doc comment.
There was a problem hiding this comment.
Another place where documenting the panic seems relevant
nomad/nomad/structs/structs.go
Line 1164 in f4cc469
There was a problem hiding this comment.
@briansorahan The panic acts as an assertion. If it is hit there is a bug in implementation. The reason it is okay is Validate should check for a valid timezone: https://github.com/hashicorp/nomad/pull/2321/files/f4cc46948f1893c3b4afc090c76f41abecefeb24#diff-396d9428285e4245041a3d0b15546d5dR1581
So the system should never even canonicalize an invalid spec.
There was a problem hiding this comment.
Are you saying that Validate guards against an invalid timezone when a periodic job is created with the CLI and HTTP?
If so then maybe my concern goes away.
I'm not very familiar with the nomad source code, but I get worried when I see a panic in code that could be part of a long-running process.
|
Thanks! The capability looks like it will do just what I was hoping for. Can't comment on implementation details though. |
|
Shouldn't this log something if it's defaulting to UTC? I don't know exactly how a bad location would get in there, and I think defaulting is sensible, but that's gonna be a heck of a confusing bug if it ever does happen. |
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR allows specification of time zones in periodic jobs.
Fixes #2276