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

refactor: use the ini_file module to test sssd.conf #67

Merged
merged 1 commit into from
Nov 13, 2023

Conversation

richm
Copy link
Contributor

@richm richm commented Nov 10, 2023

The ini lookup is problematic with ansible 2.9 due to the
arguments being completely different, and because ansible attempts
to evaluate {{ lookup('ini', ...) }} even if guarded by a when,
any use of the new style 2.x ini arguments will fail.
Instead, just use the ini_file module itself, and rely on the fact
that it will report changed: true if the values are different.

Refactor the tests_dyndns.yml test so that it works in CI with
a single host - in that case, just check that the sssd.conf was
written correctly.

The tests_dyndns.yml test wasn't working properly - in order for
any parameters to be set, you must specify ad_dyndns_update: true

Signed-off-by: Rich Megginson [email protected]

@richm richm requested a review from spetrosi as a code owner November 10, 2023 19:31
@richm
Copy link
Contributor Author

richm commented Nov 10, 2023

@jakub-vavra-cz @justin-stephenson ptal

@richm richm force-pushed the refactor-ini-handling branch from 46afc97 to 2180036 Compare November 10, 2023 19:35
@richm
Copy link
Contributor Author

richm commented Nov 10, 2023

[citest]

@justin-stephenson
Copy link
Collaborator

Refactor the tests_dyndns.yml test so that it works in CI with a single host - in that case, just check that the sssd.conf was written correctly.

I'm not QE so I will defer to @jakub-vavra-cz but I thought it would be better skip this test when AD is not available, to not give the false impression that dynamic DNS testing was successful in the case of a single host where dynamic DNS functionality is impossible.

Unrelated, one minor typo in the filename sed 's/tests_sssd_custom_setttings.yml/tests_sssd_custom_settings.yml/'

Code changes LGTM.

@richm
Copy link
Contributor Author

richm commented Nov 10, 2023

Refactor the tests_dyndns.yml test so that it works in CI with a single host - in that case, just check that the sssd.conf was written correctly.

I'm not QE so I will defer to @jakub-vavra-cz but I thought it would be better skip this test when AD is not available, to not give the false impression that dynamic DNS testing was successful in the case of a single host where dynamic DNS functionality is impossible.

Understood - but I really wanted to have a way to test the ini file handling stuff without having to set up a real AD. And I did uncover an issue with the tests_dyndns.yml

If you do have a linux and an AD host in your inventory, the test will check that DNS is configured properly.

Unrelated, one minor typo in the filename sed 's/tests_sssd_custom_setttings.yml/tests_sssd_custom_settings.yml/'

Thanks - fixed

Code changes LGTM.

@richm richm force-pushed the refactor-ini-handling branch from 2180036 to 543ab65 Compare November 10, 2023 22:49
The `ini` lookup is problematic with ansible 2.9 due to the
arguments being completely different, and because ansible attempts
to evaluate `{{ lookup('ini', ...) }}` even if guarded by a `when`,
any use of the new style 2.x `ini` arguments will fail.
Instead, just use the `ini_file` module itself, and rely on the fact
that it will report `changed: true` if the values are different.

Use `community.general.ini_file` instead of `ini_file` everywhere
for consistency.

Refactor the `tests_dyndns.yml` test so that it works in CI with
a single host - in that case, just check that the sssd.conf was
written correctly.

Rename tests_sssd_custom_setttings.yml to tests_sssd_custom_settings.yml

Use the new `ad_integration_sssd_custom_settings` in tests instead of
writing to sssd.conf directly with `ini_file`

The `tests_dyndns.yml` test wasn't working properly - in order for
any parameters to be set, you must specify `ad_dyndns_update: true`

Use non-FQCN `win_command` in a couple of places that Ansible 2.9 was
giving an error about.

Signed-off-by: Rich Megginson <[email protected]>
@richm richm force-pushed the refactor-ini-handling branch from 543ab65 to a71b826 Compare November 10, 2023 23:19
@richm
Copy link
Contributor Author

richm commented Nov 10, 2023

[citest]

@justin-stephenson
Copy link
Collaborator

justin-stephenson commented Nov 13, 2023

Ack from my side. Thank you for taking care of it.

@richm richm merged commit c540d35 into linux-system-roles:main Nov 13, 2023
8 checks passed
@richm richm deleted the refactor-ini-handling branch November 13, 2023 16:44
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