-
Notifications
You must be signed in to change notification settings - Fork 834
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
Integration test for #783 #832
Conversation
Newer verisons of /etc/sudoers prefer @includedir over #includedir. Ensure we handle that properly and don't include an additional #includedir when one isn't warranted.
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.
Thanks James! The core of this test looks sound, I have some potential cleanups and nits inline.
def test_sudoers_includedir(client: IntegrationInstance): | ||
"""Ensure we don't add additional #includedir to sudoers. | ||
|
||
Newer verisons of /etc/sudoers will use @includedir rather than |
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.
Newer verisons of /etc/sudoers will use @includedir rather than | |
Newer versions of /etc/sudoers will use @includedir rather than |
|
||
https://github.com/canonical/cloud-init/pull/783 | ||
""" | ||
if ImageSpecification.from_os_image().release not in ['hirsute', 'groovy']: |
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 looks like it'll require updating in ~2mos; should we do in ['xenial', 'bionic', 'focal']
instead? (Or perhaps @pytest.mark.not_xenial
, .not_bionic
, and .not_focal
?)
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.
Yep, good idea. I prefer the former these days because otherwise we'll have 8-10 marks specifying every current release and not-release (plus future releases), and that's just for ubuntu. In the back of my mind, I have an idea for having a single mark to specify OS/feature dependencies...but...that doesn't help us now 😄
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.
Yeah, I was just thinking that @pytest.mark.ubuntu(not=["xenial", "bionic", "focal"])
or similar would work nicely for this.
|
||
sudoers = client.read_from_file('/etc/sudoers') | ||
if '@includedir /etc/sudoers.d' not in sudoers: | ||
client.execute("echo '@includedir /etc/sudoers.d' > /etc/sudoers") |
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.
Probably a nit, but:
client.execute("echo '@includedir /etc/sudoers.d' > /etc/sudoers") | |
client.execute("echo '@includedir /etc/sudoers.d' >> /etc/sudoers") |
will avoid replacing all existing content of /etc/sudoers
: given we use sudo
to execute commands, that seems like it could backfire.
sudoers = client.read_from_file('/etc/sudoers') | ||
if '@includedir /etc/sudoers.d' not in sudoers: | ||
client.execute("echo '@includedir /etc/sudoers.d' > /etc/sudoers") | ||
client.execute('cloud-init clean --logs') |
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 is inline with other tests, so fine to keep as-is here, but should we generally be doing:
client.execute('cloud-init clean --logs') | |
client.instance.clean() |
in such cases?
if ImageSpecification.from_os_image().release in [ | ||
'xenial', 'bionic', 'focal' | ||
]: | ||
raise pytest.skip( | ||
'Test requires version of sudo installed on groovy and later' | ||
) |
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.
I'm running for the weekend, but I just realised doing this instead of marks means that we'll launch the instance before deciding that we don't want to run this test (because we're using client
and not session_cloud
as we do elsewhere with this pattern).
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.
We'll address this in #839
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.
Thanks, one minor typo fix that I'll apply now through the GH UI.
if ImageSpecification.from_os_image().release in [ | ||
'xenial', 'bionic', 'focal' | ||
]: | ||
raise pytest.skip( | ||
'Test requires version of sudo installed on groovy and later' | ||
) |
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.
We'll address this in #839
Proposed Commit Message
Additional Context
#783
Test Steps
Run tests/integration_tests/modules/test_users_groups.py on Groovy or later. Pre-SRU, you'll also need an image including the relevant code.
Checklist: