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

Fix platform check for Debian 12 #880

Merged
merged 1 commit into from
Mar 18, 2024
Merged

Conversation

wolsen
Copy link
Collaborator

@wolsen wolsen commented Mar 6, 2024

The osplatform.get_platform() check fails on Debian Bookworm because the output determined by the /etc/os-release file has "Debian GNU/Linux" and the get_platform() check only looks for lowercase debian.

This is causing tests to fail in the OpenStack Charms CI, as seen here - https://review.opendev.org/c/openstack/charm-keystone-openidc/+/908638

@wolsen wolsen force-pushed the fix-platform-check branch from 017f5b6 to bf729cd Compare March 6, 2024 13:48
The osplatform.get_platform() check fails on Debian Bookworm
because the output determined by the /etc/os-release file has
"Debian GNU/Linux" and the get_platform() check only looks for
lowercase debian.

Signed-off-by: Billy Olsen <[email protected]>
@wolsen wolsen force-pushed the fix-platform-check branch from bf729cd to 274cb37 Compare March 7, 2024 02:36
Copy link
Contributor

@fnordahl fnordahl left a comment

Choose a reason for hiding this comment

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

The approach LGTM, thanks for the extended test coverage, although I have a non-blocking nit on the unit tests.


class TestPlatform(unittest.TestCase):

@mock.patch.object(osplatform, "_get_current_platform")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: there is a BaseTestCase class in tests/utils.py that allows you to do these patches in-line as opposed to using the decorators.

The decorator approach quickly becomes hard to manage with all the positional arguments etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't disagree with the nit, but I guess for expediency, let's merge this. I think there's a larger body of work in re-working (or adding separate methods) .patch(...) and .patch_object to also/instead return the mock so that static code analysers in vscode and Neovim+LSP don't complain about the magic members that are added to the object as part of the patching functionality.

@ajkavanagh ajkavanagh merged commit 511bc03 into juju:master Mar 18, 2024
4 checks passed
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.

4 participants