[sssd PR#436][opened] subdomains: Remove code only used in tests

[amitkumar50]

   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

[sssd PR#436][comment] subdomains: Remove code only used in tests

[centos-ci]

  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

[sssd PR#436][comment] subdomains: Remove code only used in tests

[centos-ci]

  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

[sssd PR#436][comment] subdomains: Remove code only used in tests

[lslebodn]

  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

[sssd PR#436][comment] subdomains: Remove code only used in tests

[lslebodn]

  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

[sssd PR#436][+Changes requested] subdomains: Remove code only used in tests

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/436
Title: #436: subdomains: Remove code only used in tests

Label: +Changes requested

[sssd PR#436][comment] subdomains: Remove code only used in tests

[fidencio]

  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

[sssd PR#436][comment] subdomains: Remove code only used in tests

[amitkumar50]

  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

[sssd PR#436][comment] subdomains: Remove code only used in tests

[fidencio]

  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

[sssd PR#436][comment] subdomains: Remove code only used in tests

[mzidek-rh]

  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

[sssd PR#436][comment] subdomains: Remove code only used in tests

[mzidek-rh]

  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

[sssd PR#436][comment] subdomains: Remove code only used in tests

[jhrozek]

  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

[sssd PR#436][comment] subdomains: Remove code only used in tests

[amitkumar50]

  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

[sssd PR#436][comment] subdomains: Remove code only used in tests

[fidencio]

  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

[sssd PR#436][comment] subdomains: Remove code only used in tests

[jhrozek]

  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

[sssd PR#436][closed] subdomains: Remove code only used in tests

[jhrozek]

   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