-
Notifications
You must be signed in to change notification settings - Fork 208
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 ConfigureTimeZone for all Unix based platforms #1818
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan 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 |
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.
LGTM but I find such PRs hard to review. There is no test, no information on why this is needed, and no linked vendor PR (e.g., to Podman) to know where it may be used.
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.
Assuming this is related to containers/podman#21332?
In this case this is straight up breaking freebsd timezone support as the old code was used for freebsd as well and now you turn it into a NOP.
So remove the linux only tag and have to existing code work for freebsd as well.
e578857
to
96be9b2
Compare
I tried to do a test, but the function in timezone is difficult to test outside of container. |
@Luap99 PTAL |
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.
LGTM
[NO NEW TESTS NEEDED] Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
[NO NEW TESTS NEEDED]