corecfg: add "system.timezone" setting to the system settings #9102
Conversation
This commit adds support for setting the timezone via the snap configuration system. This allows image customizations in UC20 that used to be done via direct manipulation of the "writable" partition in UC16/UC18. The image customization part will be fine as is but for the runtime setting we also need to fix LP: #1650688
|
|
||
| func (s *timezoneSuite) TestConfigureTimezoneInvalid(c *C) { | ||
| invalidTimezones := []string{ | ||
| "no-#", "no-ä", "no/tripple/slash/", |
There was a problem hiding this comment.
Nitpick: I believe this should be "triple" in this context?
| } | ||
|
|
||
| var validTimezone = regexp.MustCompile(`^[a-zA-Z0-9+_-]+(/[a-zA-Z0-9+_-]+)?(/[a-zA-Z0-9+_-]+)?$`).MatchString | ||
|
|
There was a problem hiding this comment.
I validated the regexp using the timezones available in my system and it's ok now 👍 (assuming that the "right" and "posix" timezones are not valid or not meant to be used directly)
There was a problem hiding this comment.
I was wondering about that myself. Perhaps we should just use: ^[a-zA-Z0-9+_-]+(/[a-zA-Z0-9+_-]+)?(/[a-zA-Z0-9+_-]+)*$ - this would allow any number of slashes.
|
Thanks, LGTM, just left a couple of comments. |
| # timezone is set | ||
| execute_remote "cat /etc/timezone" | MATCH "Europe/Malta" | ||
| execute_remote "readlink -f /etc/localtime" | MATCH "Europe/Malta" |
There was a problem hiding this comment.
A better check would be to use install hook of this test to inspect /etc/localtime symlink, similiar to what it does for rsyslog above. This will ensure it was applied early.
| if !validTimezone(timezone) { | ||
| return fmt.Errorf("cannot set timezone %q: name not valid", timezone) |
There was a problem hiding this comment.
Go's time module has time.LoadLocation function, not sure if it's worth it but could be used as well.
location, err := time.LoadLocation("America/Los_Angeles")
if err != nil {
...
There was a problem hiding this comment.
Hmm, maybe not a good idea, it probably depends on runtime stuff that we might want to avoid e.g. with early config
There was a problem hiding this comment.
Yeah, not sure about this for exactly the reason you outlined - build-host != runtime system and all that.
| # timezone is set | ||
| execute_remote "cat /etc/timezone" | MATCH "Europe/Malta" | ||
| execute_remote "readlink -f /etc/localtime" | MATCH "Europe/Malta" | ||
| execute_remote "cat /var/snap/pc/common/debug.txt" | MATCH "localtime symlink: /usr/share/zoneinfo/Europe/Malta" |
mvo5
added
Run nested
| system: | ||
| timezone: Europe/Malta |
There was a problem hiding this comment.
unclear how this didn't break the test as merged, but the system key here is duplicated, making the core20-early-config test fail because it can't build the gadget snap.
I think this speaks to a larger issue around our nested tests in that it's unclear if when run as a PR they are actually always including snapd from the spread branch in the test. I am led to believe that they do not.
There was a problem hiding this comment.
to be clear, apparently we set BUILD_SNAPD_FROM_CURRENT to false if it is not already set, but it is set when we run nested spread tests via github actions:
Line 832 in 86feee0
and
snapd/.github/workflows/test.yaml
Line 281 in 86feee0
This commit adds support for setting the timezone via the snap
configuration system. This allows image customizations in UC20
that used to be done via direct manipulation of the "writable"
partition in UC16/UC18.
The image customization part will be fine as is but for the runtime
setting we also need to fix LP: #1650688