Skip to content

test(locale): improve test using locale en_US.UTF-8 #269

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

Merged
merged 4 commits into from
May 31, 2019

Conversation

myii
Copy link
Contributor

@myii myii commented May 14, 2019

  • Configure locale on Debian-based instances and CentOS-7

Continues work done in #262, where the locales were commented out at the time.

@myii myii requested a review from vutny May 14, 2019 15:20
@n-rodriguez
Copy link
Contributor

@myii : good work 👍

but I have one question : can the locales be set in the docker image instead?

Copy link
Contributor

@vutny vutny left a comment

Choose a reason for hiding this comment

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

In modern Debian distros and derivatives, the locale-gen just reads /etc/locale.gen file, and simply ignores locale as and argument. Despite what the manpage tells you :) So it is superfluous.

When I want to test l11n and i18n within my images I usually rely on explicitly setting the locales package configuration via debconf. That mitigates any weird hacks were done in the underlying image by someone else. Using these commands:

echo 'locales locales/locales_to_be_generated multiselect en_US.UTF-8 UTF-8, en_GB.UTF-8 UTF-8' | sudo debconf-set-selections -
echo 'locales locales/default_environment_locale select en_US.UTF-8' | sudo debconf-set-selections -
sudo dpkg-reconfigure -f noninteractive locales

This is rather a suggestion, not a mandatory thing.

@myii
Copy link
Contributor Author

myii commented May 15, 2019

@vutny Thanks for the feedback. I've tried it out on a separate branch but I'm hitting the same error on all of the Debian-based instances:

       *** update-locale: Error: invalid locale settings:  LANG=en_US.UTF-8
       The command '/bin/sh -c sudo dpkg-reconfigure -f noninteractive locales' returned a non-zero code: 255

Full logs available here: https://travis-ci.com/myii/postgres-formula/builds/111870004.


In any case, we've discussed this on Slack and @javierbertoli may be able to ensure that we don't need this process at all. en_US.UTF-8 is already configured on most of the images, except for debian-8 and debian-9. If those can be resolved, we can remove all of the provision_command sections from this PR.

@vutny
Copy link
Contributor

vutny commented May 15, 2019

@vutny Thanks for the feedback. I've tried it out on a separate branch but I'm hitting the same error on all of the Debian-based instances:

Ah, of course! Forgot the very first command 😄

sudo rm -f /etc/default/locale /etc/locale.gen

In any case, we've discussed this on Slack and @javierbertoli may be able to ensure that we don't need this process at all. en_US.UTF-8 is already configured on most of the images, except for debian-8 and debian-9. If those can be resolved, we can remove all of the provision_command sections from this PR.

Yeah, sure. This is the best option. Better to put those commands into Dockerfile.

@myii
Copy link
Contributor Author

myii commented May 15, 2019

@vutny Yes, that worked: https://travis-ci.com/myii/postgres-formula/builds/111883527. Let's see what the final outcome is for en_US.UTF-8.

@myii myii force-pushed the chore/test-locales branch from 69151ea to 2ff919f Compare May 31, 2019 01:19
@myii myii changed the title test(locale): improve test using locale en_GB.UTF-8 test(locale): improve test using locale en_US.UTF-8 May 31, 2019
@myii
Copy link
Contributor Author

myii commented May 31, 2019

Thanks to @javierbertoli, all of the pre-salted images are configured with the en_US.UTF-8 locale. So all of the provision_commands have been removed.

I've also added a couple of further changes based on the template-formula. The first one is to remove the temporary suse conditional since that's been fixed upstream (thanks @n-rodriguez!). The second is to reduce the testing matrix back to the "tree"-shaped matrix. I did test these changes before reducing the matrix and all was OK:

@vutny If you are good with the changes, would you mind merging this?

Copy link
Contributor

@vutny vutny left a comment

Choose a reason for hiding this comment

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

Thanks @myii , great work!
I guess you need to throw away changes in .travis.yml, as I see all defined instances have been passed in your other branch covered by Travis.

@myii
Copy link
Contributor Author

myii commented May 31, 2019

Thanks @myii , great work!
I guess you need to throw away changes in .travis.yml, as I see all defined instances have been passed in your other branch covered by Travis.

@vutny It's a bit ugly but we've been leaving them there (commented out) to make it easier when diffing with the template-formula (across numerous formulas). It's especially helpful when there are multiple changes to propagate, as there already have been with .travis.yml. And there are still more changes expected, since we're evaluating other CI solutions. So I'd appreciate it if we can keep hold of them there, for easier multi-formula maintenance.

@vutny vutny self-requested a review May 31, 2019 11:22
Copy link
Contributor

@vutny vutny left a comment

Choose a reason for hiding this comment

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

@myii Okay, got you.
And I hope this would be really temporary thing, since we have the full test suit working here, so be able to catch any regressions.

@myii
Copy link
Contributor Author

myii commented May 31, 2019

@vutny Sorry, I didn't get your angle about the "temporary thing". Do you mean in terms of having all of the instances running again? Or do you mean removing the commented lines? In any case, if anyone wants to help stabilise the CI setup, that would be greatly appreciated. Here's a bit more background for interested parties, even if only to offer comment: saltstack-formulas/template-formula#118.

I'll go ahead and merge this in the meanwhile, thanks for the review.

@myii myii merged commit cb4e405 into saltstack-formulas:master May 31, 2019
@myii myii deleted the chore/test-locales branch May 31, 2019 17:37
@saltstack-formulas-travis

🎉 This PR is included in version 0.37.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants