-
Notifications
You must be signed in to change notification settings - Fork 60
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
Set a default locale when building images #151
Conversation
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.
Some quibbles that I think won't make any difference in practice. I think I'd be ok merging this under the circumstances but would want to follow up fairly promptly...
return nil | ||
} | ||
|
||
err = osMkdirAll(defaultPath, 0755) |
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.
etc/default is shipped by base-files so i don't think this is necessary but it's also not harmful i guess.
defaultPath := filepath.Join(classicStateMachine.tempDirs.chroot, "etc", "default") | ||
localePath := filepath.Join(defaultPath, "locale") | ||
localeBytes, err := osReadFile(localePath) | ||
if err == nil && strings.Contains(string(localeBytes), "LANG=") { |
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.
It's a pain but this should really check for the regex "^LANG=" right? This would match "#LANG=" ...
return fmt.Errorf("Error creating default directory: %s", err.Error()) | ||
} | ||
|
||
err = osWriteFile(localePath, []byte("# Default Ubuntu locale\nLANG=C.UTF-8\n"), 0644) |
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.
This overwrites any previous file right? In practice this is extremely likely to be fine but there are ways it could incorrect I suppose, like if the image creator wants to set LC_MESSAGES to a different value to LC_COLLATE for some insane reason.
Thank you for the review Michael! I made the match taking into consideration LANG and LC_*, and also made the match from the start of the line (+ tests). Let's see if all the tests pass, if yes, I'll merge it. We can always make it better later. |
The tests are failing because of out-of-disk-space errors, but trunk is failing right now as well - so unrelated. A shame as this means there's no coverage, but I don't think I have the capacity to take care of it as part of this PR. |
Do it only in the case where no other customization set a default locale. This is to fix this bug:
https://bugs.launchpad.net/ubuntu/+source/cloud-init/+bug/2038945
livecd-rootfs was always setting a default locale of
C.UTF-8
, so we want to do the same.