Skip to content

scripts: added TZ environment variable to set timezone#2530

Merged
georglauterbach merged 9 commits intomasterfrom
scripts/timezone
Apr 6, 2022
Merged

scripts: added TZ environment variable to set timezone#2530
georglauterbach merged 9 commits intomasterfrom
scripts/timezone

Conversation

@georglauterbach
Copy link
Copy Markdown
Member

@georglauterbach georglauterbach commented Apr 4, 2022 •
edited
Loading

Description

We currently set the time zone by mounting /etc/localtime. This is fine for Docker, and users will still be able to set their time like that. This PR introduces a new way of setting date and time, that one users can use alternatively. This is especially nice when mounting from the host is difficult. Moreover, I think this is nicer, more elegant and cleaner than mounting /etc/localtime. If you prefer mounting /etc/localtime though, you can still do that.

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (README.md or the documentation under docs/)
  • If necessary I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@georglauterbach georglauterbach added kind/new feature A new feature is requested in this issue or implemeted with this PR area/scripts labels Apr 4, 2022
@georglauterbach georglauterbach added this to the v11.0.0 milestone Apr 4, 2022
@georglauterbach georglauterbach self-assigned this Apr 4, 2022
wernerfred
wernerfred reviewed Apr 4, 2022
View reviewed changes
Comment thread target/scripts/startup/setup-stack.sh Outdated
wernerfred
wernerfred previously approved these changes Apr 4, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@wernerfred wernerfred left a comment

Choose a reason for hiding this comment

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

Not familiar with debconf tzdata command but the rest seems logical to me 👍🏻

@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Apr 4, 2022 •
edited
Loading

Not familiar with debconf tzdata command but the rest seems logical to me 👍🏻

Trust me, I played around with this with the help of user-patches.sh. The debconf thingy is the most reliable way to set the system time in Debian containers :D

@casperklein
Copy link
Copy Markdown
Member

casperklein commented Apr 4, 2022 •
edited
Loading

The debconf thingy is the most reliable way to set the system time in Debian containers :D

Did you try something simple like this?

function _setup_timezone
{
  _log 'info' "Setting timezone to '${TZ}'"

  ln -fs "/usr/share/zoneinfo/${TZ}" /etc/localtime &&
  dpkg-reconfigure -f noninteractive tzdata &>/dev/null
}

If so, what problems did you encounter? That's similar to what I use and never had problems with.

PS: Your test ran successful with this one too.

casperklein
casperklein reviewed Apr 4, 2022
View reviewed changes
Comment thread docs/content/config/environment.md Outdated
Comment thread mailserver.env
Comment thread test/mail_time.bats Outdated
@casperklein casperklein mentioned this pull request Apr 4, 2022
Merged
8 tasks
wernerfred
wernerfred previously approved these changes Apr 5, 2022
View reviewed changes
@wernerfred
Copy link
Copy Markdown
Member

wernerfred commented Apr 5, 2022

If so, what problems did you encounter? That's similar to what I use and never had problems with.\n\nPS: Your test ran successful with this one too.

Does something speaks against this implementation? Easier to follow along for me (who doesnt have a clue about tzdata). Just curious.

@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Apr 5, 2022

The debconf thingy is the most reliable way to set the system time in Debian containers :D

Did you try something simple like this?

function _setup_timezone
{
  _log 'info' "Setting timezone to '${TZ}'"

  ln -fs "/usr/share/zoneinfo/${TZ}" /etc/localtime &&
  dpkg-reconfigure -f noninteractive tzdata &>/dev/null
}

It's been a long time, but I think I tried a few ways.

If so, what problems did you encounter? That's similar to what I use and never had problems with.

PS: Your test ran successful with this one too.

I have to admit, I do not remember (if there even were any).

Does something speaks against this implementation? Easier to follow along for me (who doesnt have a clue about tzdata). Just curious.

Nothing, really. I got to be honest though, I'm not sure about this because I vividly remember that some setups caused issues. BUT since tests seem to pass though as @casperklein reported, I will change it because it indeed looks easier :D

@georglauterbach
simplified TZ setup (PR suggestion)
88a1d5b 
I adjusted the Dockerfile here, and set `DEBIAN_FRONTEND` and
`DEBCONF_NONINTERACTIVE_SEEN` as ENV to have it available in the
container as well. This was done because

1. this is "a" proper way of disabling interactive deb-conf
2. especially this solution allows for non-interactive deb-conf
   everywhere without always specifying the variables / CLI options as
   well
@georglauterbach
simplified TZ setup (PR suggestion)
5af31e6 
I adjusted the Dockerfile here, and set `DEBIAN_FRONTEND` and
`DEBCONF_NONINTERACTIVE_SEEN` as ENV to have it available in the
container as well. This was done because

1. this is "a" proper way of disabling interactive deb-conf
2. especially this solution allows for non-interactive deb-conf
   everywhere without always specifying the variables / CLI options as
   well
@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Apr 5, 2022 •
edited
Loading

Not sure what happened there with 88a1d5b, but this is probably on me because I tried to push before I pulled (again...). Changes seems okay though, all is well.

wernerfred
wernerfred reviewed Apr 5, 2022
View reviewed changes
Comment thread Dockerfile Outdated
wernerfred
wernerfred reviewed Apr 5, 2022
View reviewed changes
Comment thread Dockerfile Outdated
wernerfred
wernerfred previously approved these changes Apr 5, 2022
View reviewed changes
casperklein
casperklein previously approved these changes Apr 5, 2022
View reviewed changes
@georglauterbach georglauterbach dismissed stale reviews from casperklein and wernerfred via 4d261e2 April 6, 2022 11:21
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 6, 2022

Documentation preview for this PR is ready! 🎉

Built with commit: 4d261e2

@georglauterbach georglauterbach merged commit a1726dc into master Apr 6, 2022
@georglauterbach georglauterbach deleted the scripts/timezone branch April 6, 2022 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@casperklein casperklein casperklein approved these changes

@wernerfred wernerfred wernerfred approved these changes

@polarathene polarathene Awaiting requested review from polarathene

Assignees

@georglauterbach georglauterbach

Labels

area/scripts kind/new feature A new feature is requested in this issue or implemeted with this PR

Projects

None yet

Milestone

v11.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@georglauterbach @casperklein @wernerfred @polarathene