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

Add timezone setting #777

Closed
wants to merge 1 commit into from
Closed

Conversation

LinuxSuRen
Copy link
Member

export TZ as an env variable

export TZ as an env variable

Signed-off-by: Zhao Xiaojie <zxjlwt@126.com>
@batmat
Copy link
Member

batmat commented Dec 14, 2018

Looks interesting, but could you please see to add tests? Thanks!

@LinuxSuRen
Copy link
Member Author

Do we have some documents about how to add tests? The manual test is ok.

@oleg-nenashev
Copy link
Member

Thanks @LinuxSuRen !
Apart from tests (which are optional IMHO), some documentation is needed before it is merged

Copy link
Member

@batmat batmat left a comment

Choose a reason for hiding this comment

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

I am finally unconvinced this feature is needed to achieve TZ mapping.

Please provide a use case for this. I do not think the image is designed to be rebuilt, but more to be extended by users out there. And in the latter case, adding that TZ would not be usable through image inheritance.

@@ -62,6 +62,10 @@ ENV JENKINS_UC_EXPERIMENTAL=https://updates.jenkins.io/experimental
ENV JENKINS_INCREMENTALS_REPO_MIRROR=https://repo.jenkins-ci.org/incrementals
RUN chown -R ${user} "$JENKINS_HOME" /usr/share/jenkins/ref

# setting timezone
ENV TZ=UTC
RUN ln -snf /usr/share/zoneinfo/$TZ /etc/localtime && echo $TZ > /etc/timezone
Copy link
Member

@batmat batmat Dec 20, 2018

Choose a reason for hiding this comment

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

Also, I tend to think this is not how we should do this: using a RUN step, this means one has to rebuild the whole image to be able to adapt the TZ.

I did that in the past using the following mapping.
What does this PR provide that cannot be provided mapping the TZ file when executing docker run?

date
docker run jenkins/jenkins:lts date
docker run -v /usr/share/zoneinfo/CET:/etc/localtime jenkins/jenkins:lts date
Thu Dec 20 14:52:37 CET 2018
Thu Dec 20 13:52:37 UTC 2018
Thu Dec 20 14:52:38 CET 2018

Copy link

@vbricard vbricard Dec 21, 2018

Choose a reason for hiding this comment

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

+1
I agree but I rather map the server's file /etc/localtime.
You should do this in docker-compose.yml

    volumes:
    - /etc/timezone:/etc/timezone:ro
    - /etc/localtime:/etc/localtime:ro

Copy link
Member Author

Choose a reason for hiding this comment

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

map for the timezone is a good solution. but it seams dependency the host os. What about other os, e.g. Window.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's in scope here, given we do not provide native Docker images for Windows, at least yet.

@batmat
Copy link
Member

batmat commented Jan 11, 2019

Going to close this given the discussion we had above.

@LinuxSuRen please feel free to come and explain your use case, so we can reopen/consider this, but I really feel like the right path to handle timezones is #777 (comment)

Having to rebuild the image to change the timezone seems much more complex than just mapping the right timezone file, which I think is also the correct way to do that in general, even apart from the context of the Jenkins Docker image.

Thanks a lot for the contribution @LinuxSuRen, please keep them coming! ❤️

@batmat batmat closed this Jan 11, 2019
@LinuxSuRen
Copy link
Member Author

@batmat Yes, I just wish I could find a better solution to setting the timezone. The mapping way still is complex.

@batmat
Copy link
Member

batmat commented Jan 13, 2019

I tend to disagree. At least, I could agree it's possibly a bit complex but still way less than rebuilding the whole image to change it :)

@LinuxSuRen
Copy link
Member Author

I agree with that the rebuilding is bad. My expect way is that docker run jenkins/jenkins -e TZ=UTC.

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.

None yet

4 participants