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

feat: Add sssd custom settings #64

Merged
merged 23 commits into from
Nov 10, 2023

Conversation

brakkio86
Copy link
Contributor

Enhancement: allow sssd custom settings

Reason: sssd.conf is fixed and not costumizable

Result:

Issue Tracker Tickets (Jira or BZ if any):

@richm
Copy link
Contributor

richm commented Nov 2, 2023

Thanks - looks useful - @justin-stephenson wdyt?

since ad_integration_sssd_custom_settings is part of the public interface, it needs the following:

  • defined in defaults/main.yml with a default value e.g. ad_integration_sssd_custom_settings: []
  • described in README.md
  • some sort of test for this - specify ad_integration_sssd_custom_settings, confirm that they are in sssd.conf

@justin-stephenson
Copy link
Collaborator

I am good with allowing custom sssd options for this role.

@brakkio86
Copy link
Contributor Author

brakkio86 commented Nov 2, 2023

Hello @richm,
I've added the default section and described in readme. What do you mean with "some sort of test for this - specify ad_integration_sssd_custom_settings, confirm that they are in sssd.conf"? In my opinion, module community.general.ini_file already do that. Or do you mean something else?

@richm
Copy link
Contributor

richm commented Nov 2, 2023

Hello @richm, I've added the default section and described in readme. What do you mean with "some sort of test for this - specify ad_integration_sssd_custom_settings, confirm that they are in sssd.conf"? In my opinion, module community.general.ini_file already do that. Or do you mean something else?

For example - take a look at https://github.com/linux-system-roles/ad_integration/blob/main/tests/tests_dyndns.yml - in fact, IMO you could change this test - add ad_integration_sssd_custom_settings: with one setting to vars:, and check for it in the Test - Verify sssd.conf options were written task. I realize it may seem like we are testing the workings of the ini_file module, but our QE is going to want to ensure that the parameter was indeed written to the config file, and would strongly prefer an automated way to do that, rather than having to manually inspect the resulting file.

@brakkio86
Copy link
Contributor Author

brakkio86 commented Nov 6, 2023

Hello, I've wrote a test. @richm what do you think?

@brakkio86
Copy link
Contributor Author

I've modified the code in order to remove the specified custom parameters. Is it ok now?

tasks/main.yml Outdated Show resolved Hide resolved
@richm richm changed the title Add sssd custom settings feat: Add sssd custom settings Nov 9, 2023
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: Richard Megginson <richm@stanfordalumni.org>
brakkio86 and others added 4 commits November 9, 2023 20:52
Co-authored-by: Richard Megginson <richm@stanfordalumni.org>
Co-authored-by: Richard Megginson <richm@stanfordalumni.org>
Co-authored-by: Richard Megginson <richm@stanfordalumni.org>
README.md Outdated Show resolved Hide resolved
@richm
Copy link
Contributor

richm commented Nov 9, 2023

[citest]

@richm
Copy link
Contributor

richm commented Nov 9, 2023

In addition to the other suggested changes, change handlers/main.yml like this:

   loop: "{{ __ad_integration_services }}"
+  when: not __ad_integration_test_sssd_config_only | d(false)

that way the handler notifications won't actually attempt to restart realmd or sssd when the config files are changed, and we only want to test the config files

brakkio86 and others added 5 commits November 10, 2023 07:19
Co-authored-by: Richard Megginson <richm@stanfordalumni.org>
Co-authored-by: Richard Megginson <richm@stanfordalumni.org>
Co-authored-by: Richard Megginson <richm@stanfordalumni.org>
Co-authored-by: Richard Megginson <richm@stanfordalumni.org>
Co-authored-by: Richard Megginson <richm@stanfordalumni.org>
@brakkio86
Copy link
Contributor Author

@richm What is the porpouse of the variable __ad_integration_test_sssd_config_only?

@brakkio86 brakkio86 closed this Nov 10, 2023
@brakkio86
Copy link
Contributor Author

In addition to the other suggested changes, change handlers/main.yml like this:

   loop: "{{ __ad_integration_services }}"
+  when: not __ad_integration_test_sssd_config_only | d(false)

that way the handler notifications won't actually attempt to restart realmd or sssd when the config files are changed, and we only want to test the config files

Done as you suggest.

@brakkio86 brakkio86 reopened this Nov 10, 2023
@brakkio86
Copy link
Contributor Author

@richm What is the porpouse of the variable __ad_integration_test_sssd_config_only?

Ignore it, I found the answer by myself

@richm
Copy link
Contributor

richm commented Nov 10, 2023

[citest]

@richm
Copy link
Contributor

richm commented Nov 10, 2023

note - I'm going to merge this even though ansible 2.9 tests are broken - the problem is that the ini lookup works differently in 2.9 and later, and 2.9 tries to evaluate the 2.x lookup call, even if excluded by a when - so I need to figure out how we will test this with 2.9 (and this applies to the dyndns support as well - I will refactor that too).

@richm richm merged commit 0796843 into linux-system-roles:main Nov 10, 2023
16 of 23 checks passed
@richm
Copy link
Contributor

richm commented Dec 12, 2023

@brakkio86 can you give me some examples of settings that you need to set in sssd.conf in order for ad_integration to work in your environment? We would like to add some tests for this feature, but we want to know some settings to apply to sssd.conf

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.

None yet

3 participants