URL: https://github.com/SSSD/sssd/pull/851 Author: alexal Title: #851: Update __init__.py.in Action: opened
PR body: """ COMPONENT: SSSDConfig
The default value for sudo_provider, auth_provider, and autofs_provider will be the value of id_provider, if those options weren't set in the configuration file
Resolves: https://pagure.io/SSSD/sssd/issue/3995 """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/851/head:pr851 git checkout pr851
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
centos-ci commented: """ Can one of the admins verify this patch? """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-512504391
URL: https://github.com/SSSD/sssd/pull/851 Author: alexal Title: #851: Update __init__.py.in Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/851/head:pr851 git checkout pr851
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexal commented: """ I do see only this error in fedora28/ci-mock-build.log
Mock Version: 1.4.15 INFO: Mock Version: 1.4.15 Start: dnf install ERROR: Exception(rpmbuild/SRPMS/sssd-2.2.1-0.fc28.src.rpm) Config(fedora-28-x86_64) 0 minutes 51 seconds INFO: Results and/or logs in: ci-mock-result INFO: Cleaning up build root ('cleanup_on_failure=True') Start: clean chroot Finish: clean chroot ERROR: Command failed: # /usr/bin/dnf --installroot /var/lib/mock/fedora-28-x86_64/root/ --releasever 28 --setopt=deltarpm=False --disableplugin=local --disableplugin=spacewalk install @buildsys-build fedora 2.8 MB/s | 60 MB 00:21 Error: Failed to synchronize cache for repo 'updates'
End: Wed Jul 17 21:59:22 UTC 2019 """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-512606230
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexey-tikhonov commented: """ Hi @alexal,
Thank you for contribution.
Would you please merge two patches of `__init__.py.in` into one? Or is there any reason to keep it separate? """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-512769474
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexey-tikhonov commented: """ Actually it looks like it is better to have all three patches as single commit. """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-512772037
URL: https://github.com/SSSD/sssd/pull/851 Author: alexal Title: #851: Update __init__.py.in Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/851/head:pr851 git checkout pr851
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexal commented: """ @alexey-tikhonov done, sorry I didn't do that from the beginning. """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-512773893
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexey-tikhonov commented: """
@alexey-tikhonov done, sorry I didn't do that from the beginning.
But now there are 5 commits in the https://github.com/alexal/sssd/commits/dev branch...
I am not sure what would be the best way to handle this, but I would: * format a single patch * reset your branch locally to the tip your started from * apply and commit patch as a single commit with meaningful commit message (*) * `push --force` your local branch to your github repo [I am sure there are more efficient git ways to do it, but this should work]
*) commit message as in 72b52b9f9caaf4862c8da5017983470e66be02e3 is fine, but please, take a note in the "COMPONENT: SSSDConfig" part in the commit template "COMPONENT" should be replaced with component being changed ("SSSDConfig" in this case) and after ":" there should be short summary. """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-512781437
URL: https://github.com/SSSD/sssd/pull/851 Author: alexal Title: #851: Update __init__.py.in Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/851/head:pr851 git checkout pr851
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexal commented: """ @alexey-tikhonov thanks for your suggestions. What about now? Looks better? """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-512794563
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexey-tikhonov commented: """
@alexey-tikhonov thanks for your suggestions. What about now? Looks better?
Much better. Thank you. """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-512807862
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexal commented: """ @alexey-tikhonov thanks for your help. """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-512808110
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexey-tikhonov commented: """ OK to test """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-512808215
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
thalman commented: """ OK to test """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-512811541
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
pbrezina commented: """ ok to test """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-512818844
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexey-tikhonov commented: """ OK to test """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-512808215
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
jhrozek commented: """ @alexey-tikhonov all the CI engines are green now. Unless you have more comments, would you mind adding the Accepted label so that we can push the PR? """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-513904165
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
jhrozek commented: """ Oh and about the centos CI triggers. I added both Alexey and Tomas to the centos CI whitelist so that the "OK to test" magic phrase would work for them. Honestly I thought everyone was added to the whitelist a long time ago.. """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-513904659
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexey-tikhonov commented: """
@alexey-tikhonov all the CI engines are green now. Unless you have more comments, would you mind adding the Accepted label so that we can push the PR?
I am sorry I didn't actually review this patch. I only helped to form it properly. """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-514170228
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexal commented: """ Who will test these changes? """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-515435317
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
jhrozek commented: """ I thought @pbrezina might? """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-516357806
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
pbrezina commented: """ When we are touching the code, I believe also `subdomains_provider` is missing. Also most of the providers defaults to `id_provider`, few have other defaults so I think it should be handled here as well. Please, see `man sssd.conf` for `*_provider` options and add what is missing. """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-516750056
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
jhrozek commented: """ I don't know if it's easy or possible but wouldn't it be better to amend the config API to internally synthetize the provider values, but does not write them to the config file? Otherwise I'm sure we will forget when another provider is added.. """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-516767396
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
pbrezina commented: """ Sure it would be better, but I'd say it is out of scope of this pull request. Unless the author wants to do the extra job, of course.
@alexal Alex, can you please complete the patch by adding missing things per [this](https://github.com/SSSD/sssd/pull/851#issuecomment-516750056) comment? Thank you. """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-522973049
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/851 Author: alexal Title: #851: Update __init__.py.in Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/851/head:pr851 git checkout pr851
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexal commented: """
@alexal Alex, can you please complete the patch by adding missing things per [this](https://github.com/SSSD/sssd/pull/851#issuecomment-516750056) comment? Thank you.
@pbrezina done. I apologize for the delay, been busy. """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-523104320
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexal commented: """
When we are touching the code, I believe also `subdomains_provider` is missing. Also most of the providers defaults to `id_provider`, few have other defaults so I think it should be handled here as well. Please, see `man sssd.conf` for `*_provider` options and add what is missing.
I've added:
- selinux_provider - hostid_provider - subdomains_provider """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-523104773
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexal commented: """ @pbrezina do you have any additional requests for changes or everything looks good? """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-523909076
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
pbrezina commented: """ Yes, some providers are still not handled (session, chpass and access). Also could you change the code to use dictionary and for loop instead of duplicating the code? Additionally I think it is not needed to call `domain.add_provider(id_provider_value, "auth")` (or is it?), but amend the `provider` dictionary as its going to set the options on next line. Something like this:
```python # Handle default provider values when not set. default_providers = { 'id_provider': None, 'auth_provider': 'id_provider', 'chpass_provider': 'auth_provider', 'sudo_provider': 'id_provider', 'selinux_provider': 'id_provider', 'subdomains_provider': 'id_provider', 'session_provider': 'id_provider', 'autofs_provider': 'id_provider', 'hostid_provider': 'id_provider' }
if 'access_provider' not in providers: providers['access_provider'] = 'permit'
for provider, default_provider in default_providers.items(): if provider in providers: continue
value = providers.get(default_provider, None) providers[provider] = value ```
I did not test this. Perhaps the `None` case needs to be handled differently. """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-527062588
URL: https://github.com/SSSD/sssd/pull/851 Author: alexal Title: #851: Update __init__.py.in Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/851/head:pr851 git checkout pr851
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexal commented: """ @pbrezina ,
I apologize for the delay, I had a lot of things at work. I've modified the code, but a few additional changes might be required, because I'm getting this error for the following providers:
- selinux_provider - subdomains_provider - session_provider
Traceback (most recent call last):
File "sssdconfig_test.py", line 6, in <module> domain = config.get_domain('EXAMPLE.COM') File "/usr/lib/python3.7/site-packages/SSSDConfig/__init__.py", line 1952, in get_domain domain.set_option(option, value) File "/usr/lib/python3.7/site-packages/SSSDConfig/__init__.py", line 1262, in set_option self.add_provider(value, provider) File "/usr/lib/python3.7/site-packages/SSSDConfig/__init__.py", line 1315, in add_provider raise NoSuchProviderSubtypeError(provider_type) SSSDConfig.NoSuchProviderSubtypeError: selinux
It does appear I have to limit default values for these provides and maybe for others too.
Thanks, Alex """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-538778833
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
Label: +Waiting for review
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
pbrezina commented: """ I also get the following error with the reproducer from the ticket description:
```bash $ sudo ./sssdconfig.py Traceback (most recent call last): File "./sssdconfig.py", line 7, in <module> domain = config.get_domain('ldap.vm') File "/usr/lib/python2.7/site-packages/SSSDConfig/__init__.py", line 1942, in get_domain value = providers[providers_list.index(default_provider)] ValueError: 'auth_provider' is not in list ```
So yes, some additional changes will be needed. """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-539498177
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
Label: -Waiting for review
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexal commented: """ @pbrezina sure, let me make those changes. """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-539500230
URL: https://github.com/SSSD/sssd/pull/851 Author: alexal Title: #851: Update __init__.py.in Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/851/head:pr851 git checkout pr851
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexal commented: """ @pbrezina ,
I've made changes to the code, all tests went fine except this:
====================================================================== FAIL: testImportConfigNoVersion (__main__.SSSDConfigTestSSSDConfig)
Traceback (most recent call last): File "/shared/sssd/src/config/SSSDConfigTest.py", line 1423, in testImportConfigNoVersion self.assertTrue(option in domain_control_list) AssertionError: False is not true
That test has failed because sssd/src/config/testconfigs/sssd-noversion.conf has no values for options shown below and we can't set "ad" as the value for them:
- selinux_provider - hostid_provider - session_provider
Should I remove them from the domain_control_list of sssd/src/config/SSSDConfigTest.py file? """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-539612380
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexal commented: """ @pbrezina ,
Just wondering if you had a chance to review the code?
Thanks! """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-541138115
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
pbrezina commented: """ No, the test fails because you are missing `chpass_provider` in `domain_control_list`. """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-542154975
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
pbrezina commented: """ So basically, you want to move those unsupported providers into `negative_domain_control_list` to make sure they are not set. And add `chpass_provider` to `domain_control_list` to make sure it is set. """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-542156240
URL: https://github.com/SSSD/sssd/pull/851 Author: alexal Title: #851: Update __init__.py.in Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/851/head:pr851 git checkout pr851
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexal commented: """ @pbrezina good catch, do not know how I missed that provider in the test. I no longer have any failures on my machine:
============================================================================ Testsuite summary for sssd 2.2.3 ============================================================================ # TOTAL: 4 # PASS: 4 # SKIP: 0 # XFAIL: 0 # FAIL: 0 # XPASS: 0 # ERROR: 0 ============================================================================ """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-542192968
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexal commented: """ @pbrezina good catch, do not know how I missed that provider in the test. I no longer have any failures on my machine:
Testsuite summary for sssd 2.2.3 # TOTAL: 4 # PASS: 4 # SKIP: 0 # XFAIL: 0 # FAIL: 0 # XPASS: 0 # ERROR: 0 """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-542192968
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexal commented: """ @pbrezina good catch, do not know how I missed that provider in the test. I no longer have any failures on my machine:
Testsuite summary for sssd 2.2.3 TOTAL: 4 PASS: 4 SKIP: 0 XFAIL: 0 FAIL: 0 XPASS: 0 ERROR: 0 """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-542192968
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
pbrezina commented: """ Thank you. One more thing - then I will ack it. Can you add a test case for the use case in ticket description? Perhaps as part of sssd-valid.conf tests. """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-542562994
URL: https://github.com/SSSD/sssd/pull/851 Author: alexal Title: #851: Update __init__.py.in Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/851/head:pr851 git checkout pr851
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexal commented: """ @pbrezina thanks! I've added ldap_sudo_include_regexp & ldap_autofs_map_master_name options to the test. That should cover everything described in the original ticket. I've run multiple tests on my machine, without my modification that test will fail, with those modifications test will work just fine. """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-543277910
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
pbrezina commented: """ Thank you. Ack.
I'm going to squash the following change (which was requested in [this comment](https://github.com/SSSD/sssd/pull/851#issuecomment-542156240)) before pushing - unless you disagree.
```diff --- a/src/config/SSSDConfigTest.py +++ b/src/config/SSSDConfigTest.py @@ -1410,9 +1410,6 @@ class SSSDConfigTestSSSDConfig(unittest.TestCase): 'chpass_provider', 'sudo_provider', 'subdomains_provider', - 'selinux_provider', - 'hostid_provider', - 'session_provider', 'default_shell', 'fallback_homedir', 'cache_credentials', @@ -1428,6 +1425,9 @@ class SSSDConfigTestSSSDConfig(unittest.TestCase): 'ad_server', 'ldap_id_mapping', 'ldap_sasl_authid', + 'selinux_provider', + 'hostid_provider', + 'session_provider' ]
for option in ad_domain.get_all_options(): ``` """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-544424178
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
Label: +Ready to push
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
alexal commented: """ @pbrezina sure, I do not mind. Thanks a lot. """
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-544469285
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
pbrezina commented: """ * `master` * 05c078e60f87d0eae33f4d6ff34a413976075b8c - Update __init__.py.in
"""
See the full comment at https://github.com/SSSD/sssd/pull/851#issuecomment-544474472
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/851 Title: #851: Update __init__.py.in
Label: -Ready to push
URL: https://github.com/SSSD/sssd/pull/851 Author: alexal Title: #851: Update __init__.py.in Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/851/head:pr851 git checkout pr851
sssd-devel@lists.fedorahosted.org