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

archlinux: Fix broken locale logic #796

Closed
wants to merge 1 commit into from
Closed

archlinux: Fix broken locale logic #796

wants to merge 1 commit into from

Conversation

klausenbusk
Copy link
Contributor

Proposed Commit Message

archlinux: Fix broken locale logic

The locale wasn't persisted correct nor set.

LP: https://bugs.launchpad.net/cloud-init/+bug/1402406

Additional Context

See LP bug: https://bugs.launchpad.net/cloud-init/+bug/1402406

Test Steps

This has been tested manually and /etc/locale.gen is in the expected format now and the locale is written to /etc/locale.conf.

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

I've verified that the CLA has been signed. Thanks for this PR.

I left a couple comments inline. Additionally, can you provide a unit test for this behavior? There's a very small unit test file at unittests/test_distros/test_arch.py . Ensuring we write out the correct locale config would be a very useful unit test.

cloudinit/distros/arch.py Show resolved Hide resolved
cloudinit/distros/arch.py Show resolved Hide resolved
@klausenbusk
Copy link
Contributor Author

There's a very small unit test file at unittests/test_distros/test_arch.py . Ensuring we write out the correct locale config would be a very useful unit test.

localectl requires systemd-localed to be running (communicating over D-Bus). Is that something which can be tested in the CI?

@TheRealFalcon
Copy link
Member

localectl requires systemd-localed to be running (communicating over D-Bus). Is that something which can be tested in the CI?

We use mocks in our unit tests. For testing this particular functionality, it would make sense to have tests that show:

  • /etc/locale.gen contains what we expect
  • localectl set-locale is called with the right argument (showing that it worked correctly would be beyond the scope of the test)
  • Invalid user input is handled appropriately (per our conversation above)

I earlier linked you to some Arch tests, but looking again, it'd probably make more sense for them to go into the test_handler_locale tests . There are also other examples in there showing how some of the mocking works.

I hope that all makes sense. Let me know if you have any questions or need some additional guidance. Further test guidelines can be found here:
https://cloudinit.readthedocs.io/en/latest/topics/testing.html

@github-actions
Copy link

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging mitechie, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag mitechie to reopen it.)

@github-actions github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Feb 23, 2021
@klausenbusk
Copy link
Contributor Author

Real life is getting in the way and writing tests is a bit more complicated than writing plain Python code. I hope to find some time in the near future.

@github-actions github-actions bot closed this Mar 5, 2021
@klausenbusk
Copy link
Contributor Author

klausenbusk commented Mar 11, 2021

@mitechie can you reopen?

@TheRealFalcon I pushed the unit test code but the PR is apparently not updating when it is closed.

@TheRealFalcon TheRealFalcon removed the stale-pr Pull request is stale; will be auto-closed soon label Mar 11, 2021
@TheRealFalcon
Copy link
Member

@klausenbusk , unfortunately it appears that github doesn't allow reopening of PRs that have had their branch force-pushed to.

Feel free to open a new PR with your changes.

@klausenbusk
Copy link
Contributor Author

@klausenbusk , unfortunately it appears that github doesn't allow reopening of PRs that have had their branch force-pushed to.

TIL. I opened a new PR: #841

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.

2 participants