URL: https://github.com/SSSD/sssd/pull/436 Author: amitkumar50 Title: #436: subdomains: Remove code only used in tests Action: opened
PR body: """ When setting the ad_options in subdomain code there is an condition if (cdb == NULL || conf_path == NULL) True branch is only usable in tests that use old code to initialize the options without confdb context. It would be good to remove dead code. Since cdb, conf_path NULL check is already done inside ad_create_options()
Resolves: https://pagure.io/SSSD/sssd/issue/3338 """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/436/head:pr436 git checkout pr436
URL: https://github.com/SSSD/sssd/pull/436 Title: #436: subdomains: Remove code only used in tests
centos-ci commented: """ Can one of the admins verify this patch? """
See the full comment at https://github.com/SSSD/sssd/pull/436#issuecomment-341670959
URL: https://github.com/SSSD/sssd/pull/436 Title: #436: subdomains: Remove code only used in tests
centos-ci commented: """ Can one of the admins verify this patch? """
See the full comment at https://github.com/SSSD/sssd/pull/436#issuecomment-341670960
URL: https://github.com/SSSD/sssd/pull/436 Title: #436: subdomains: Remove code only used in tests
lslebodn commented: """ ok to test """
See the full comment at https://github.com/SSSD/sssd/pull/436#issuecomment-341718678
URL: https://github.com/SSSD/sssd/pull/436 Title: #436: subdomains: Remove code only used in tests
lslebodn commented: """ @amitkumar50 Removing code is not enough otherwise unit test will fail. You need to also update related test.
``` FAIL: ad_common_tests =====================
[==========] Running 11 test(s). [ RUN ] test_ad_create_1way_trust_options (Fri Nov 3 15:50:25:508078 2017) [sssd] [confdb_get_param] (0x0020): Failed to get [ldap_uri] from [(null)], error [22] (Invalid argument) (Fri Nov 3 15:50:25:508104 2017) [sssd] [confdb_get_string] (0x0020): Failed to get [ldap_uri] from [(null)], error [22] (Invalid argument) (Fri Nov 3 15:50:25:508109 2017) [sssd] [dp_get_options] (0x0020): Failed to retrieve value for option (ldap_uri) (Fri Nov 3 15:50:25:508119 2017) [sssd] [ad_create_1way_trust_options] (0x0020): ad_create_options failed Could not run the test - check test fixtures [ ERROR ] test_ad_create_1way_trust_options [ RUN ] test_ad_create_2way_trust_options (Fri Nov 3 15:50:25:512705 2017) [sssd] [confdb_get_param] (0x0020): Failed to get [ldap_uri] from [(null)], error [22] (Invalid argument) (Fri Nov 3 15:50:25:512715 2017) [sssd] [confdb_get_string] (0x0020): Failed to get [ldap_uri] from [(null)], error [22] (Invalid argument) (Fri Nov 3 15:50:25:512719 2017) [sssd] [dp_get_options] (0x0020): Failed to retrieve value for option (ldap_uri) (Fri Nov 3 15:50:25:512723 2017) [sssd] [ad_create_2way_trust_options] (0x0020): ad_create_options failed Could not run the test - check test fixtures [ ERROR ] test_ad_create_2way_trust_options ``` """
See the full comment at https://github.com/SSSD/sssd/pull/436#issuecomment-341725811
URL: https://github.com/SSSD/sssd/pull/436 Title: #436: subdomains: Remove code only used in tests
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/436 Title: #436: subdomains: Remove code only used in tests
fidencio commented: """ @amitkumar50, please, when contributing to the project, always try to: - run `$ make check` in your patches - run `$ make intgcheck`
And in case you're fixing a bug, point to a reproducer that the reviewer can see (it may be either be here in github or to the pagure issue or red hat bugzilla).
Thanks a lot for your contribution and we're looking forward to seeing the new version of the patch """
See the full comment at https://github.com/SSSD/sssd/pull/436#issuecomment-341758722
URL: https://github.com/SSSD/sssd/pull/436 Title: #436: subdomains: Remove code only used in tests
amitkumar50 commented: """ @fidencio Thanks for comments. Which specific tests to remove for, can you please guide. """
See the full comment at https://github.com/SSSD/sssd/pull/436#issuecomment-342783368
URL: https://github.com/SSSD/sssd/pull/436 Title: #436: subdomains: Remove code only used in tests
fidencio commented: """ @amitkumar50, you'll have to take a loot at least at: - test_ad_create_1way_trust_options - test_ad_create_2way_trust_options
Keep in mind that we do **not** want to remove those tests. What we want to have is a way to remove the code path just used there and adapt the tests to this change.
I don't have much info about this, but I suspect that @mzidek-rh has and he'd be the best person to guide you through this task.
@mzidek-rh, please, feel free to throw some ideas here on what's the best way that you'd expect to have this issue solved. """
See the full comment at https://github.com/SSSD/sssd/pull/436#issuecomment-342791042
URL: https://github.com/SSSD/sssd/pull/436 Title: #436: subdomains: Remove code only used in tests
mzidek-rh commented: """ Hi, the whole point of the issue is to update the test to use confdb and stop using the code path that automatically generates the options, because that code path is no longer used in SSSD and it was only kept around for the sake of the tests. """
See the full comment at https://github.com/SSSD/sssd/pull/436#issuecomment-342800269
URL: https://github.com/SSSD/sssd/pull/436 Title: #436: subdomains: Remove code only used in tests
mzidek-rh commented: """ Hi, the whole point of the issue is to update the tests (that fail with this patch) to use confdb and stop using the code path that automatically generates the options, because that code path is no longer used in SSSD and it was only kept around for the sake of these tests. """
See the full comment at https://github.com/SSSD/sssd/pull/436#issuecomment-342800269
URL: https://github.com/SSSD/sssd/pull/436 Title: #436: subdomains: Remove code only used in tests
jhrozek commented: """ @amitkumar50 ping, do you still plan on updating this PR? """
See the full comment at https://github.com/SSSD/sssd/pull/436#issuecomment-349311997
URL: https://github.com/SSSD/sssd/pull/436 Title: #436: subdomains: Remove code only used in tests
amitkumar50 commented: """ @jhrozek Yes I plan to update. But still not clear which tests to remove from test_ad_create_1way_trust_options(void **state) and test_ad_create_2way_trust_options """
See the full comment at https://github.com/SSSD/sssd/pull/436#issuecomment-349320709
URL: https://github.com/SSSD/sssd/pull/436 Title: #436: subdomains: Remove code only used in tests
fidencio commented: """ @amitkumar50, you don't have to **remove** tests from test_ad_create_1way_trust_options and test_ad_create_2way_trust_options. What you'll have to do is dig in the code and find out what has to be done in order to have those tests passing with your patch.
So, in other words, this bug is not only about removing a piece of code, but also to update the tests properly in the way that the removed piece of code is not needed in the tests.
Quoting @mzidek-rh here: You'll need to "use confdb and stop using the code path that automatically generates the options, because that code path is no longer used in SSSD and it was only kept around for the sake of these tests.". """
See the full comment at https://github.com/SSSD/sssd/pull/436#issuecomment-349334562
URL: https://github.com/SSSD/sssd/pull/436 Title: #436: subdomains: Remove code only used in tests
jhrozek commented: """ Hi @amitkumar50, since this PR has been stalled for quite some time, I'm going to close it. Please feel free to reopen the PR if you plan on addressing the issues that were brought up. Also, please feel free to work with us on the #sssd IRC channel if you need some help. """
See the full comment at https://github.com/SSSD/sssd/pull/436#issuecomment-390787841
URL: https://github.com/SSSD/sssd/pull/436 Author: amitkumar50 Title: #436: subdomains: Remove code only used in tests Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/436/head:pr436 git checkout pr436
sssd-devel@lists.fedorahosted.org