Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust
On 18.07.2016 18:07, Martin Babinsky wrote: On 07/18/2016 04:59 PM, Lenka Doudova wrote: On 07/18/2016 04:55 PM, Martin Babinsky wrote: On 07/14/2016 11:42 AM, Lenka Doudova wrote: On 07/13/2016 05:40 PM, Martin Babinsky wrote: On 07/01/2016 04:15 PM, Lenka Doudova wrote: On 07/01/2016 02:38 PM, Martin Babinsky wrote: On 07/01/2016 06:36 AM, Lenka Doudova wrote: On 06/30/2016 05:01 PM, Martin Babinsky wrote: On 06/30/2016 03:47 PM, Lenka Doudova wrote: Hi, attaching patch with some basic coverage for external trust feature. Bit more detailed info in commit message. Since the feature requires me to run commands previously used only for forest root domains even for subdomains, I made some changes in ipatests/test_integration/tasks.py file, so that it would enable me to reuse existing function without copy-pasting them for one variable change. Lenka Hi Lenka, I have a few comments: 1.) -def establish_trust_with_ad(master, ad, extra_args=()): +def establish_trust_with_ad(master, ad, extra_args=(), subdomain=False): This modification seems extremely kludgy to me. I would rather change the function signature to: -def establish_trust_with_ad(master, ad, extra_args=()): +def establish_trust_with_ad(master, ad_domain, extra_args=()): and pass the domain name itself to the function instead of doing magic inside. The same goes with `remove trust_with_ad`. You may want to fix the calls to them in existing code so that the domain is passed instead of the whole trust object. 2.) +def sync_time_hostname(host, srv_hostname): +""" +Syncs time with the remote server given by its hostname. +Please note that this function leaves ntpd stopped. +""" +host.run_command(['systemctl', 'stop', 'ntpd']) +host.run_command(['ntpdate', srv_hostname]) + + this looks like a duplicate of the function above it, why is it even needed? 3.) +class TestExternalTrustWithSubdomain(ADTrustBase): +""" +Test establishing external trust with subdomain +""" + +@classmethod +def install(cls, mh): +super(ADTrustBase, cls).install(mh) +cls.ad = cls.ad_domains[0].ads[0] +cls.install_adtrust() +cls.check_sid_generation() + +# Determine whether the subdomain AD is available +try: +child_ad = cls.host_by_role(cls.optional_extra_roles[0]) +cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:]) +cls.ad_subdomain_hostname = child_ad.hostname +except LookupError: +cls.ad_subdomain = None + +cls.configure_dns_and_time() Please use proper OOP practices when overriding the behavior of the base test class. For starters you could rewrite the install method like this: +@classmethod +def install(cls, mh): +# Determine whether the subdomain AD is available +try: +cls.child_ad = cls.host_by_role(cls.optional_extra_roles[0]) +cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:]) +cls.ad_subdomain_hostname = child_ad.hostname +except LookupError: +raise nose.SkipTest("AFAIK this will skip the whole test class") +super(ADTrustBase, cls).install(mh) with child_ad stored as class attribute, you can override `configure_dns_and_time`: +def configure_dns_and_time(cls): +tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain) # no need to re-implement the function just to get to the child AD DC hostname now +tasks.sync_time(cls.master, cls.child_ad.hostname) You can then use this class as an intermediary in the hierarchy and inherit the other external/non-external trust test classes from it, since most setup method in them are just copy-pasted from this one. Hi, thanks for review, fixed patch attached. Lenka Hi Lenka, I am still not happy about the patch underutilizing the potential for a proper inheritance hierarchy and instead relying on a staggering amounts of copy-pasting for implementation. I am attaching a quick untested patch illustrating how the implementation should look like. Also you have some leftovers from previous revision, please fix them: Pylint is running, please wait ... * Module ipatests.test_integration.test_trust ipatests/test_integration/test_trust.py:236: [E1101(no-member), TestExternalTrustWithSubdomain.configure_dns_and_time] Module 'ipatests.test_integration.tasks' has no 'sync_time_hostname' member) ipatests/test_integration/test_trust.py:243: [E1123(unexpected-keyword-arg), TestExternalTrustWithSubdomain.test_establish_trust] Unexpected keyword argument 'subdomain' in function call) ipatests/test_integration/test_trust.py:279: [E1123(unexpected-keyword-arg), TestExternalTrustWithSubdomain.test_remove_nonposix_trust] Unexpected keyword argument 'subdomain' in function call) ipatests/test_integration/test_trust.py:330: [E1101(no-member), TestNonexternalTrustWithSubdomain.configure_dns_and
Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust
On 07/18/2016 04:59 PM, Lenka Doudova wrote: On 07/18/2016 04:55 PM, Martin Babinsky wrote: On 07/14/2016 11:42 AM, Lenka Doudova wrote: On 07/13/2016 05:40 PM, Martin Babinsky wrote: On 07/01/2016 04:15 PM, Lenka Doudova wrote: On 07/01/2016 02:38 PM, Martin Babinsky wrote: On 07/01/2016 06:36 AM, Lenka Doudova wrote: On 06/30/2016 05:01 PM, Martin Babinsky wrote: On 06/30/2016 03:47 PM, Lenka Doudova wrote: Hi, attaching patch with some basic coverage for external trust feature. Bit more detailed info in commit message. Since the feature requires me to run commands previously used only for forest root domains even for subdomains, I made some changes in ipatests/test_integration/tasks.py file, so that it would enable me to reuse existing function without copy-pasting them for one variable change. Lenka Hi Lenka, I have a few comments: 1.) -def establish_trust_with_ad(master, ad, extra_args=()): +def establish_trust_with_ad(master, ad, extra_args=(), subdomain=False): This modification seems extremely kludgy to me. I would rather change the function signature to: -def establish_trust_with_ad(master, ad, extra_args=()): +def establish_trust_with_ad(master, ad_domain, extra_args=()): and pass the domain name itself to the function instead of doing magic inside. The same goes with `remove trust_with_ad`. You may want to fix the calls to them in existing code so that the domain is passed instead of the whole trust object. 2.) +def sync_time_hostname(host, srv_hostname): +""" +Syncs time with the remote server given by its hostname. +Please note that this function leaves ntpd stopped. +""" +host.run_command(['systemctl', 'stop', 'ntpd']) +host.run_command(['ntpdate', srv_hostname]) + + this looks like a duplicate of the function above it, why is it even needed? 3.) +class TestExternalTrustWithSubdomain(ADTrustBase): +""" +Test establishing external trust with subdomain +""" + +@classmethod +def install(cls, mh): +super(ADTrustBase, cls).install(mh) +cls.ad = cls.ad_domains[0].ads[0] +cls.install_adtrust() +cls.check_sid_generation() + +# Determine whether the subdomain AD is available +try: +child_ad = cls.host_by_role(cls.optional_extra_roles[0]) +cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:]) +cls.ad_subdomain_hostname = child_ad.hostname +except LookupError: +cls.ad_subdomain = None + +cls.configure_dns_and_time() Please use proper OOP practices when overriding the behavior of the base test class. For starters you could rewrite the install method like this: +@classmethod +def install(cls, mh): +# Determine whether the subdomain AD is available +try: +cls.child_ad = cls.host_by_role(cls.optional_extra_roles[0]) +cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:]) +cls.ad_subdomain_hostname = child_ad.hostname +except LookupError: +raise nose.SkipTest("AFAIK this will skip the whole test class") +super(ADTrustBase, cls).install(mh) with child_ad stored as class attribute, you can override `configure_dns_and_time`: +def configure_dns_and_time(cls): +tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain) # no need to re-implement the function just to get to the child AD DC hostname now +tasks.sync_time(cls.master, cls.child_ad.hostname) You can then use this class as an intermediary in the hierarchy and inherit the other external/non-external trust test classes from it, since most setup method in them are just copy-pasted from this one. Hi, thanks for review, fixed patch attached. Lenka Hi Lenka, I am still not happy about the patch underutilizing the potential for a proper inheritance hierarchy and instead relying on a staggering amounts of copy-pasting for implementation. I am attaching a quick untested patch illustrating how the implementation should look like. Also you have some leftovers from previous revision, please fix them: Pylint is running, please wait ... * Module ipatests.test_integration.test_trust ipatests/test_integration/test_trust.py:236: [E1101(no-member), TestExternalTrustWithSubdomain.configure_dns_and_time] Module 'ipatests.test_integration.tasks' has no 'sync_time_hostname' member) ipatests/test_integration/test_trust.py:243: [E1123(unexpected-keyword-arg), TestExternalTrustWithSubdomain.test_establish_trust] Unexpected keyword argument 'subdomain' in function call) ipatests/test_integration/test_trust.py:279: [E1123(unexpected-keyword-arg), TestExternalTrustWithSubdomain.test_remove_nonposix_trust] Unexpected keyword argument 'subdomain' in function call) ipatests/test_integration/test_trust.py:330: [E1101(no-member), TestNonexternalTrustWithSubdomain.configure_dns_and_time] Module 'ipatests.test_integration.tasks' has no 'sync_time
Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust
On 07/18/2016 04:55 PM, Martin Babinsky wrote: On 07/14/2016 11:42 AM, Lenka Doudova wrote: On 07/13/2016 05:40 PM, Martin Babinsky wrote: On 07/01/2016 04:15 PM, Lenka Doudova wrote: On 07/01/2016 02:38 PM, Martin Babinsky wrote: On 07/01/2016 06:36 AM, Lenka Doudova wrote: On 06/30/2016 05:01 PM, Martin Babinsky wrote: On 06/30/2016 03:47 PM, Lenka Doudova wrote: Hi, attaching patch with some basic coverage for external trust feature. Bit more detailed info in commit message. Since the feature requires me to run commands previously used only for forest root domains even for subdomains, I made some changes in ipatests/test_integration/tasks.py file, so that it would enable me to reuse existing function without copy-pasting them for one variable change. Lenka Hi Lenka, I have a few comments: 1.) -def establish_trust_with_ad(master, ad, extra_args=()): +def establish_trust_with_ad(master, ad, extra_args=(), subdomain=False): This modification seems extremely kludgy to me. I would rather change the function signature to: -def establish_trust_with_ad(master, ad, extra_args=()): +def establish_trust_with_ad(master, ad_domain, extra_args=()): and pass the domain name itself to the function instead of doing magic inside. The same goes with `remove trust_with_ad`. You may want to fix the calls to them in existing code so that the domain is passed instead of the whole trust object. 2.) +def sync_time_hostname(host, srv_hostname): +""" +Syncs time with the remote server given by its hostname. +Please note that this function leaves ntpd stopped. +""" +host.run_command(['systemctl', 'stop', 'ntpd']) +host.run_command(['ntpdate', srv_hostname]) + + this looks like a duplicate of the function above it, why is it even needed? 3.) +class TestExternalTrustWithSubdomain(ADTrustBase): +""" +Test establishing external trust with subdomain +""" + +@classmethod +def install(cls, mh): +super(ADTrustBase, cls).install(mh) +cls.ad = cls.ad_domains[0].ads[0] +cls.install_adtrust() +cls.check_sid_generation() + +# Determine whether the subdomain AD is available +try: +child_ad = cls.host_by_role(cls.optional_extra_roles[0]) +cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:]) +cls.ad_subdomain_hostname = child_ad.hostname +except LookupError: +cls.ad_subdomain = None + +cls.configure_dns_and_time() Please use proper OOP practices when overriding the behavior of the base test class. For starters you could rewrite the install method like this: +@classmethod +def install(cls, mh): +# Determine whether the subdomain AD is available +try: +cls.child_ad = cls.host_by_role(cls.optional_extra_roles[0]) +cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:]) +cls.ad_subdomain_hostname = child_ad.hostname +except LookupError: +raise nose.SkipTest("AFAIK this will skip the whole test class") +super(ADTrustBase, cls).install(mh) with child_ad stored as class attribute, you can override `configure_dns_and_time`: +def configure_dns_and_time(cls): +tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain) # no need to re-implement the function just to get to the child AD DC hostname now +tasks.sync_time(cls.master, cls.child_ad.hostname) You can then use this class as an intermediary in the hierarchy and inherit the other external/non-external trust test classes from it, since most setup method in them are just copy-pasted from this one. Hi, thanks for review, fixed patch attached. Lenka Hi Lenka, I am still not happy about the patch underutilizing the potential for a proper inheritance hierarchy and instead relying on a staggering amounts of copy-pasting for implementation. I am attaching a quick untested patch illustrating how the implementation should look like. Also you have some leftovers from previous revision, please fix them: Pylint is running, please wait ... * Module ipatests.test_integration.test_trust ipatests/test_integration/test_trust.py:236: [E1101(no-member), TestExternalTrustWithSubdomain.configure_dns_and_time] Module 'ipatests.test_integration.tasks' has no 'sync_time_hostname' member) ipatests/test_integration/test_trust.py:243: [E1123(unexpected-keyword-arg), TestExternalTrustWithSubdomain.test_establish_trust] Unexpected keyword argument 'subdomain' in function call) ipatests/test_integration/test_trust.py:279: [E1123(unexpected-keyword-arg), TestExternalTrustWithSubdomain.test_remove_nonposix_trust] Unexpected keyword argument 'subdomain' in function call) ipatests/test_integration/test_trust.py:330: [E1101(no-member), TestNonexternalTrustWithSubdomain.configure_dns_and_time] Module 'ipatests.test_integration.tasks' has no 'sync_time_hostname' member) Makefile:137: reci
Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust
On 07/14/2016 11:42 AM, Lenka Doudova wrote: On 07/13/2016 05:40 PM, Martin Babinsky wrote: On 07/01/2016 04:15 PM, Lenka Doudova wrote: On 07/01/2016 02:38 PM, Martin Babinsky wrote: On 07/01/2016 06:36 AM, Lenka Doudova wrote: On 06/30/2016 05:01 PM, Martin Babinsky wrote: On 06/30/2016 03:47 PM, Lenka Doudova wrote: Hi, attaching patch with some basic coverage for external trust feature. Bit more detailed info in commit message. Since the feature requires me to run commands previously used only for forest root domains even for subdomains, I made some changes in ipatests/test_integration/tasks.py file, so that it would enable me to reuse existing function without copy-pasting them for one variable change. Lenka Hi Lenka, I have a few comments: 1.) -def establish_trust_with_ad(master, ad, extra_args=()): +def establish_trust_with_ad(master, ad, extra_args=(), subdomain=False): This modification seems extremely kludgy to me. I would rather change the function signature to: -def establish_trust_with_ad(master, ad, extra_args=()): +def establish_trust_with_ad(master, ad_domain, extra_args=()): and pass the domain name itself to the function instead of doing magic inside. The same goes with `remove trust_with_ad`. You may want to fix the calls to them in existing code so that the domain is passed instead of the whole trust object. 2.) +def sync_time_hostname(host, srv_hostname): +""" +Syncs time with the remote server given by its hostname. +Please note that this function leaves ntpd stopped. +""" +host.run_command(['systemctl', 'stop', 'ntpd']) +host.run_command(['ntpdate', srv_hostname]) + + this looks like a duplicate of the function above it, why is it even needed? 3.) +class TestExternalTrustWithSubdomain(ADTrustBase): +""" +Test establishing external trust with subdomain +""" + +@classmethod +def install(cls, mh): +super(ADTrustBase, cls).install(mh) +cls.ad = cls.ad_domains[0].ads[0] +cls.install_adtrust() +cls.check_sid_generation() + +# Determine whether the subdomain AD is available +try: +child_ad = cls.host_by_role(cls.optional_extra_roles[0]) +cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:]) +cls.ad_subdomain_hostname = child_ad.hostname +except LookupError: +cls.ad_subdomain = None + +cls.configure_dns_and_time() Please use proper OOP practices when overriding the behavior of the base test class. For starters you could rewrite the install method like this: +@classmethod +def install(cls, mh): +# Determine whether the subdomain AD is available +try: +cls.child_ad = cls.host_by_role(cls.optional_extra_roles[0]) +cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:]) +cls.ad_subdomain_hostname = child_ad.hostname +except LookupError: +raise nose.SkipTest("AFAIK this will skip the whole test class") +super(ADTrustBase, cls).install(mh) with child_ad stored as class attribute, you can override `configure_dns_and_time`: +def configure_dns_and_time(cls): +tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain) # no need to re-implement the function just to get to the child AD DC hostname now +tasks.sync_time(cls.master, cls.child_ad.hostname) You can then use this class as an intermediary in the hierarchy and inherit the other external/non-external trust test classes from it, since most setup method in them are just copy-pasted from this one. Hi, thanks for review, fixed patch attached. Lenka Hi Lenka, I am still not happy about the patch underutilizing the potential for a proper inheritance hierarchy and instead relying on a staggering amounts of copy-pasting for implementation. I am attaching a quick untested patch illustrating how the implementation should look like. Also you have some leftovers from previous revision, please fix them: Pylint is running, please wait ... * Module ipatests.test_integration.test_trust ipatests/test_integration/test_trust.py:236: [E1101(no-member), TestExternalTrustWithSubdomain.configure_dns_and_time] Module 'ipatests.test_integration.tasks' has no 'sync_time_hostname' member) ipatests/test_integration/test_trust.py:243: [E1123(unexpected-keyword-arg), TestExternalTrustWithSubdomain.test_establish_trust] Unexpected keyword argument 'subdomain' in function call) ipatests/test_integration/test_trust.py:279: [E1123(unexpected-keyword-arg), TestExternalTrustWithSubdomain.test_remove_nonposix_trust] Unexpected keyword argument 'subdomain' in function call) ipatests/test_integration/test_trust.py:330: [E1101(no-member), TestNonexternalTrustWithSubdomain.configure_dns_and_time] Module 'ipatests.test_integration.tasks' has no 'sync_time_hostname' member) Makefile:137: recipe for target 'lint' failed make: *** [lint] Error 1 sendin
Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust
On 07/13/2016 05:40 PM, Martin Babinsky wrote: On 07/01/2016 04:15 PM, Lenka Doudova wrote: On 07/01/2016 02:38 PM, Martin Babinsky wrote: On 07/01/2016 06:36 AM, Lenka Doudova wrote: On 06/30/2016 05:01 PM, Martin Babinsky wrote: On 06/30/2016 03:47 PM, Lenka Doudova wrote: Hi, attaching patch with some basic coverage for external trust feature. Bit more detailed info in commit message. Since the feature requires me to run commands previously used only for forest root domains even for subdomains, I made some changes in ipatests/test_integration/tasks.py file, so that it would enable me to reuse existing function without copy-pasting them for one variable change. Lenka Hi Lenka, I have a few comments: 1.) -def establish_trust_with_ad(master, ad, extra_args=()): +def establish_trust_with_ad(master, ad, extra_args=(), subdomain=False): This modification seems extremely kludgy to me. I would rather change the function signature to: -def establish_trust_with_ad(master, ad, extra_args=()): +def establish_trust_with_ad(master, ad_domain, extra_args=()): and pass the domain name itself to the function instead of doing magic inside. The same goes with `remove trust_with_ad`. You may want to fix the calls to them in existing code so that the domain is passed instead of the whole trust object. 2.) +def sync_time_hostname(host, srv_hostname): +""" +Syncs time with the remote server given by its hostname. +Please note that this function leaves ntpd stopped. +""" +host.run_command(['systemctl', 'stop', 'ntpd']) +host.run_command(['ntpdate', srv_hostname]) + + this looks like a duplicate of the function above it, why is it even needed? 3.) +class TestExternalTrustWithSubdomain(ADTrustBase): +""" +Test establishing external trust with subdomain +""" + +@classmethod +def install(cls, mh): +super(ADTrustBase, cls).install(mh) +cls.ad = cls.ad_domains[0].ads[0] +cls.install_adtrust() +cls.check_sid_generation() + +# Determine whether the subdomain AD is available +try: +child_ad = cls.host_by_role(cls.optional_extra_roles[0]) +cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:]) +cls.ad_subdomain_hostname = child_ad.hostname +except LookupError: +cls.ad_subdomain = None + +cls.configure_dns_and_time() Please use proper OOP practices when overriding the behavior of the base test class. For starters you could rewrite the install method like this: +@classmethod +def install(cls, mh): +# Determine whether the subdomain AD is available +try: +cls.child_ad = cls.host_by_role(cls.optional_extra_roles[0]) +cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:]) +cls.ad_subdomain_hostname = child_ad.hostname +except LookupError: +raise nose.SkipTest("AFAIK this will skip the whole test class") +super(ADTrustBase, cls).install(mh) with child_ad stored as class attribute, you can override `configure_dns_and_time`: +def configure_dns_and_time(cls): +tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain) # no need to re-implement the function just to get to the child AD DC hostname now +tasks.sync_time(cls.master, cls.child_ad.hostname) You can then use this class as an intermediary in the hierarchy and inherit the other external/non-external trust test classes from it, since most setup method in them are just copy-pasted from this one. Hi, thanks for review, fixed patch attached. Lenka Hi Lenka, I am still not happy about the patch underutilizing the potential for a proper inheritance hierarchy and instead relying on a staggering amounts of copy-pasting for implementation. I am attaching a quick untested patch illustrating how the implementation should look like. Also you have some leftovers from previous revision, please fix them: Pylint is running, please wait ... * Module ipatests.test_integration.test_trust ipatests/test_integration/test_trust.py:236: [E1101(no-member), TestExternalTrustWithSubdomain.configure_dns_and_time] Module 'ipatests.test_integration.tasks' has no 'sync_time_hostname' member) ipatests/test_integration/test_trust.py:243: [E1123(unexpected-keyword-arg), TestExternalTrustWithSubdomain.test_establish_trust] Unexpected keyword argument 'subdomain' in function call) ipatests/test_integration/test_trust.py:279: [E1123(unexpected-keyword-arg), TestExternalTrustWithSubdomain.test_remove_nonposix_trust] Unexpected keyword argument 'subdomain' in function call) ipatests/test_integration/test_trust.py:330: [E1101(no-member), TestNonexternalTrustWithSubdomain.configure_dns_and_time] Module 'ipatests.test_integration.tasks' has no 'sync_time_hostname' member) Makefile:137: recipe for target 'lint' failed make: *** [lint] Error 1 sending incremental file list (this is befor
Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust
On 07/01/2016 04:15 PM, Lenka Doudova wrote: On 07/01/2016 02:38 PM, Martin Babinsky wrote: On 07/01/2016 06:36 AM, Lenka Doudova wrote: On 06/30/2016 05:01 PM, Martin Babinsky wrote: On 06/30/2016 03:47 PM, Lenka Doudova wrote: Hi, attaching patch with some basic coverage for external trust feature. Bit more detailed info in commit message. Since the feature requires me to run commands previously used only for forest root domains even for subdomains, I made some changes in ipatests/test_integration/tasks.py file, so that it would enable me to reuse existing function without copy-pasting them for one variable change. Lenka Hi Lenka, I have a few comments: 1.) -def establish_trust_with_ad(master, ad, extra_args=()): +def establish_trust_with_ad(master, ad, extra_args=(), subdomain=False): This modification seems extremely kludgy to me. I would rather change the function signature to: -def establish_trust_with_ad(master, ad, extra_args=()): +def establish_trust_with_ad(master, ad_domain, extra_args=()): and pass the domain name itself to the function instead of doing magic inside. The same goes with `remove trust_with_ad`. You may want to fix the calls to them in existing code so that the domain is passed instead of the whole trust object. 2.) +def sync_time_hostname(host, srv_hostname): +""" +Syncs time with the remote server given by its hostname. +Please note that this function leaves ntpd stopped. +""" +host.run_command(['systemctl', 'stop', 'ntpd']) +host.run_command(['ntpdate', srv_hostname]) + + this looks like a duplicate of the function above it, why is it even needed? 3.) +class TestExternalTrustWithSubdomain(ADTrustBase): +""" +Test establishing external trust with subdomain +""" + +@classmethod +def install(cls, mh): +super(ADTrustBase, cls).install(mh) +cls.ad = cls.ad_domains[0].ads[0] +cls.install_adtrust() +cls.check_sid_generation() + +# Determine whether the subdomain AD is available +try: +child_ad = cls.host_by_role(cls.optional_extra_roles[0]) +cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:]) +cls.ad_subdomain_hostname = child_ad.hostname +except LookupError: +cls.ad_subdomain = None + +cls.configure_dns_and_time() Please use proper OOP practices when overriding the behavior of the base test class. For starters you could rewrite the install method like this: +@classmethod +def install(cls, mh): +# Determine whether the subdomain AD is available +try: +cls.child_ad = cls.host_by_role(cls.optional_extra_roles[0]) +cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:]) +cls.ad_subdomain_hostname = child_ad.hostname +except LookupError: +raise nose.SkipTest("AFAIK this will skip the whole test class") +super(ADTrustBase, cls).install(mh) with child_ad stored as class attribute, you can override `configure_dns_and_time`: +def configure_dns_and_time(cls): +tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain) # no need to re-implement the function just to get to the child AD DC hostname now +tasks.sync_time(cls.master, cls.child_ad.hostname) You can then use this class as an intermediary in the hierarchy and inherit the other external/non-external trust test classes from it, since most setup method in them are just copy-pasted from this one. Hi, thanks for review, fixed patch attached. Lenka Hi Lenka, I am still not happy about the patch underutilizing the potential for a proper inheritance hierarchy and instead relying on a staggering amounts of copy-pasting for implementation. I am attaching a quick untested patch illustrating how the implementation should look like. Also you have some leftovers from previous revision, please fix them: Pylint is running, please wait ... * Module ipatests.test_integration.test_trust ipatests/test_integration/test_trust.py:236: [E1101(no-member), TestExternalTrustWithSubdomain.configure_dns_and_time] Module 'ipatests.test_integration.tasks' has no 'sync_time_hostname' member) ipatests/test_integration/test_trust.py:243: [E1123(unexpected-keyword-arg), TestExternalTrustWithSubdomain.test_establish_trust] Unexpected keyword argument 'subdomain' in function call) ipatests/test_integration/test_trust.py:279: [E1123(unexpected-keyword-arg), TestExternalTrustWithSubdomain.test_remove_nonposix_trust] Unexpected keyword argument 'subdomain' in function call) ipatests/test_integration/test_trust.py:330: [E1101(no-member), TestNonexternalTrustWithSubdomain.configure_dns_and_time] Module 'ipatests.test_integration.tasks' has no 'sync_time_hostname' member) Makefile:137: recipe for target 'lint' failed make: *** [lint] Error 1 sending incremental file list (this is before I started meddling with it) Thank you very much for
Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust
On 07/01/2016 02:38 PM, Martin Babinsky wrote: On 07/01/2016 06:36 AM, Lenka Doudova wrote: On 06/30/2016 05:01 PM, Martin Babinsky wrote: On 06/30/2016 03:47 PM, Lenka Doudova wrote: Hi, attaching patch with some basic coverage for external trust feature. Bit more detailed info in commit message. Since the feature requires me to run commands previously used only for forest root domains even for subdomains, I made some changes in ipatests/test_integration/tasks.py file, so that it would enable me to reuse existing function without copy-pasting them for one variable change. Lenka Hi Lenka, I have a few comments: 1.) -def establish_trust_with_ad(master, ad, extra_args=()): +def establish_trust_with_ad(master, ad, extra_args=(), subdomain=False): This modification seems extremely kludgy to me. I would rather change the function signature to: -def establish_trust_with_ad(master, ad, extra_args=()): +def establish_trust_with_ad(master, ad_domain, extra_args=()): and pass the domain name itself to the function instead of doing magic inside. The same goes with `remove trust_with_ad`. You may want to fix the calls to them in existing code so that the domain is passed instead of the whole trust object. 2.) +def sync_time_hostname(host, srv_hostname): +""" +Syncs time with the remote server given by its hostname. +Please note that this function leaves ntpd stopped. +""" +host.run_command(['systemctl', 'stop', 'ntpd']) +host.run_command(['ntpdate', srv_hostname]) + + this looks like a duplicate of the function above it, why is it even needed? 3.) +class TestExternalTrustWithSubdomain(ADTrustBase): +""" +Test establishing external trust with subdomain +""" + +@classmethod +def install(cls, mh): +super(ADTrustBase, cls).install(mh) +cls.ad = cls.ad_domains[0].ads[0] +cls.install_adtrust() +cls.check_sid_generation() + +# Determine whether the subdomain AD is available +try: +child_ad = cls.host_by_role(cls.optional_extra_roles[0]) +cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:]) +cls.ad_subdomain_hostname = child_ad.hostname +except LookupError: +cls.ad_subdomain = None + +cls.configure_dns_and_time() Please use proper OOP practices when overriding the behavior of the base test class. For starters you could rewrite the install method like this: +@classmethod +def install(cls, mh): +# Determine whether the subdomain AD is available +try: +cls.child_ad = cls.host_by_role(cls.optional_extra_roles[0]) +cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:]) +cls.ad_subdomain_hostname = child_ad.hostname +except LookupError: +raise nose.SkipTest("AFAIK this will skip the whole test class") +super(ADTrustBase, cls).install(mh) with child_ad stored as class attribute, you can override `configure_dns_and_time`: +def configure_dns_and_time(cls): +tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain) # no need to re-implement the function just to get to the child AD DC hostname now +tasks.sync_time(cls.master, cls.child_ad.hostname) You can then use this class as an intermediary in the hierarchy and inherit the other external/non-external trust test classes from it, since most setup method in them are just copy-pasted from this one. Hi, thanks for review, fixed patch attached. Lenka Hi Lenka, I am still not happy about the patch underutilizing the potential for a proper inheritance hierarchy and instead relying on a staggering amounts of copy-pasting for implementation. I am attaching a quick untested patch illustrating how the implementation should look like. Also you have some leftovers from previous revision, please fix them: Pylint is running, please wait ... * Module ipatests.test_integration.test_trust ipatests/test_integration/test_trust.py:236: [E1101(no-member), TestExternalTrustWithSubdomain.configure_dns_and_time] Module 'ipatests.test_integration.tasks' has no 'sync_time_hostname' member) ipatests/test_integration/test_trust.py:243: [E1123(unexpected-keyword-arg), TestExternalTrustWithSubdomain.test_establish_trust] Unexpected keyword argument 'subdomain' in function call) ipatests/test_integration/test_trust.py:279: [E1123(unexpected-keyword-arg), TestExternalTrustWithSubdomain.test_remove_nonposix_trust] Unexpected keyword argument 'subdomain' in function call) ipatests/test_integration/test_trust.py:330: [E1101(no-member), TestNonexternalTrustWithSubdomain.configure_dns_and_time] Module 'ipatests.test_integration.tasks' has no 'sync_time_hostname' member) Makefile:137: recipe for target 'lint' failed make: *** [lint] Error 1 sending incremental file list (this is before I started meddling with it) Thank you very much for the example, fixed patch
Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust
On 07/01/2016 06:36 AM, Lenka Doudova wrote: On 06/30/2016 05:01 PM, Martin Babinsky wrote: On 06/30/2016 03:47 PM, Lenka Doudova wrote: Hi, attaching patch with some basic coverage for external trust feature. Bit more detailed info in commit message. Since the feature requires me to run commands previously used only for forest root domains even for subdomains, I made some changes in ipatests/test_integration/tasks.py file, so that it would enable me to reuse existing function without copy-pasting them for one variable change. Lenka Hi Lenka, I have a few comments: 1.) -def establish_trust_with_ad(master, ad, extra_args=()): +def establish_trust_with_ad(master, ad, extra_args=(), subdomain=False): This modification seems extremely kludgy to me. I would rather change the function signature to: -def establish_trust_with_ad(master, ad, extra_args=()): +def establish_trust_with_ad(master, ad_domain, extra_args=()): and pass the domain name itself to the function instead of doing magic inside. The same goes with `remove trust_with_ad`. You may want to fix the calls to them in existing code so that the domain is passed instead of the whole trust object. 2.) +def sync_time_hostname(host, srv_hostname): +""" +Syncs time with the remote server given by its hostname. +Please note that this function leaves ntpd stopped. +""" +host.run_command(['systemctl', 'stop', 'ntpd']) +host.run_command(['ntpdate', srv_hostname]) + + this looks like a duplicate of the function above it, why is it even needed? 3.) +class TestExternalTrustWithSubdomain(ADTrustBase): +""" +Test establishing external trust with subdomain +""" + +@classmethod +def install(cls, mh): +super(ADTrustBase, cls).install(mh) +cls.ad = cls.ad_domains[0].ads[0] +cls.install_adtrust() +cls.check_sid_generation() + +# Determine whether the subdomain AD is available +try: +child_ad = cls.host_by_role(cls.optional_extra_roles[0]) +cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:]) +cls.ad_subdomain_hostname = child_ad.hostname +except LookupError: +cls.ad_subdomain = None + +cls.configure_dns_and_time() Please use proper OOP practices when overriding the behavior of the base test class. For starters you could rewrite the install method like this: +@classmethod +def install(cls, mh): +# Determine whether the subdomain AD is available +try: +cls.child_ad = cls.host_by_role(cls.optional_extra_roles[0]) +cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:]) +cls.ad_subdomain_hostname = child_ad.hostname +except LookupError: +raise nose.SkipTest("AFAIK this will skip the whole test class") +super(ADTrustBase, cls).install(mh) with child_ad stored as class attribute, you can override `configure_dns_and_time`: +def configure_dns_and_time(cls): +tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain) # no need to re-implement the function just to get to the child AD DC hostname now +tasks.sync_time(cls.master, cls.child_ad.hostname) You can then use this class as an intermediary in the hierarchy and inherit the other external/non-external trust test classes from it, since most setup method in them are just copy-pasted from this one. Hi, thanks for review, fixed patch attached. Lenka Hi Lenka, I am still not happy about the patch underutilizing the potential for a proper inheritance hierarchy and instead relying on a staggering amounts of copy-pasting for implementation. I am attaching a quick untested patch illustrating how the implementation should look like. Also you have some leftovers from previous revision, please fix them: Pylint is running, please wait ... * Module ipatests.test_integration.test_trust ipatests/test_integration/test_trust.py:236: [E1101(no-member), TestExternalTrustWithSubdomain.configure_dns_and_time] Module 'ipatests.test_integration.tasks' has no 'sync_time_hostname' member) ipatests/test_integration/test_trust.py:243: [E1123(unexpected-keyword-arg), TestExternalTrustWithSubdomain.test_establish_trust] Unexpected keyword argument 'subdomain' in function call) ipatests/test_integration/test_trust.py:279: [E1123(unexpected-keyword-arg), TestExternalTrustWithSubdomain.test_remove_nonposix_trust] Unexpected keyword argument 'subdomain' in function call) ipatests/test_integration/test_trust.py:330: [E1101(no-member), TestNonexternalTrustWithSubdomain.configure_dns_and_time] Module 'ipatests.test_integration.tasks' has no 'sync_time_hostname' member) Makefile:137: recipe for target 'lint' failed make: *** [lint] Error 1 sending incremental file list (this is before I started meddling with it) -- Martin^3 Babinsky From f9849e9667e595d67ffd631f312106ae4278c135 Mon Sep 17 00:00:00 2001 From: Martin
Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust
On 06/30/2016 05:01 PM, Martin Babinsky wrote: On 06/30/2016 03:47 PM, Lenka Doudova wrote: Hi, attaching patch with some basic coverage for external trust feature. Bit more detailed info in commit message. Since the feature requires me to run commands previously used only for forest root domains even for subdomains, I made some changes in ipatests/test_integration/tasks.py file, so that it would enable me to reuse existing function without copy-pasting them for one variable change. Lenka Hi Lenka, I have a few comments: 1.) -def establish_trust_with_ad(master, ad, extra_args=()): +def establish_trust_with_ad(master, ad, extra_args=(), subdomain=False): This modification seems extremely kludgy to me. I would rather change the function signature to: -def establish_trust_with_ad(master, ad, extra_args=()): +def establish_trust_with_ad(master, ad_domain, extra_args=()): and pass the domain name itself to the function instead of doing magic inside. The same goes with `remove trust_with_ad`. You may want to fix the calls to them in existing code so that the domain is passed instead of the whole trust object. 2.) +def sync_time_hostname(host, srv_hostname): +""" +Syncs time with the remote server given by its hostname. +Please note that this function leaves ntpd stopped. +""" +host.run_command(['systemctl', 'stop', 'ntpd']) +host.run_command(['ntpdate', srv_hostname]) + + this looks like a duplicate of the function above it, why is it even needed? 3.) +class TestExternalTrustWithSubdomain(ADTrustBase): +""" +Test establishing external trust with subdomain +""" + +@classmethod +def install(cls, mh): +super(ADTrustBase, cls).install(mh) +cls.ad = cls.ad_domains[0].ads[0] +cls.install_adtrust() +cls.check_sid_generation() + +# Determine whether the subdomain AD is available +try: +child_ad = cls.host_by_role(cls.optional_extra_roles[0]) +cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:]) +cls.ad_subdomain_hostname = child_ad.hostname +except LookupError: +cls.ad_subdomain = None + +cls.configure_dns_and_time() Please use proper OOP practices when overriding the behavior of the base test class. For starters you could rewrite the install method like this: +@classmethod +def install(cls, mh): +# Determine whether the subdomain AD is available +try: +cls.child_ad = cls.host_by_role(cls.optional_extra_roles[0]) +cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:]) +cls.ad_subdomain_hostname = child_ad.hostname +except LookupError: +raise nose.SkipTest("AFAIK this will skip the whole test class") +super(ADTrustBase, cls).install(mh) with child_ad stored as class attribute, you can override `configure_dns_and_time`: +def configure_dns_and_time(cls): +tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain) # no need to re-implement the function just to get to the child AD DC hostname now +tasks.sync_time(cls.master, cls.child_ad.hostname) You can then use this class as an intermediary in the hierarchy and inherit the other external/non-external trust test classes from it, since most setup method in them are just copy-pasted from this one. Hi, thanks for review, fixed patch attached. Lenka From ae860f97d701b296f52de1eb0c84dd43d1429f76 Mon Sep 17 00:00:00 2001 From: Lenka Doudova Date: Thu, 30 Jun 2016 12:23:02 +0200 Subject: [PATCH] Tests: External trust Provides basic coverage for external trust feature. Test cases: 1. verify an external trust with AD subdomain can be established - verify only one trustdomain is listed - verify subdomain users are resolvable - verify trust can be deleted 2. verify non-external trust with AD subdomain cannot be established 3. verify an external trust with AD forest root domain can be established - verify that even if AD subdomain is specified, it is not associated with the trust - verify trust can be deleted https://fedorahosted.org/freeipa/ticket/5743 --- ipatests/test_integration/tasks.py | 11 +- ipatests/test_integration/test_trust.py | 218 2 files changed, 197 insertions(+), 32 deletions(-) diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py index 5be7cdae3ac777bbf0fc52e6c511969e9fabcd72..48ef6f1963e2405d7db9a18799e9796b7f10bfe5 100644 --- a/ipatests/test_integration/tasks.py +++ b/ipatests/test_integration/tasks.py @@ -443,7 +443,7 @@ def configure_dns_for_trust(master, ad): pass -def establish_trust_with_ad(master, ad, extra_args=()): +def establish_trust_with_ad(master, ad_domain, extra_args=()): """ Establishes trust with Active Directory. Trust type is detected depending on the presence of SfU (Services for Unix) support on
Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust
On 06/30/2016 03:47 PM, Lenka Doudova wrote: Hi, attaching patch with some basic coverage for external trust feature. Bit more detailed info in commit message. Since the feature requires me to run commands previously used only for forest root domains even for subdomains, I made some changes in ipatests/test_integration/tasks.py file, so that it would enable me to reuse existing function without copy-pasting them for one variable change. Lenka Hi Lenka, I have a few comments: 1.) -def establish_trust_with_ad(master, ad, extra_args=()): +def establish_trust_with_ad(master, ad, extra_args=(), subdomain=False): This modification seems extremely kludgy to me. I would rather change the function signature to: -def establish_trust_with_ad(master, ad, extra_args=()): +def establish_trust_with_ad(master, ad_domain, extra_args=()): and pass the domain name itself to the function instead of doing magic inside. The same goes with `remove trust_with_ad`. You may want to fix the calls to them in existing code so that the domain is passed instead of the whole trust object. 2.) +def sync_time_hostname(host, srv_hostname): +""" +Syncs time with the remote server given by its hostname. +Please note that this function leaves ntpd stopped. +""" +host.run_command(['systemctl', 'stop', 'ntpd']) +host.run_command(['ntpdate', srv_hostname]) + + this looks like a duplicate of the function above it, why is it even needed? 3.) +class TestExternalTrustWithSubdomain(ADTrustBase): +""" +Test establishing external trust with subdomain +""" + +@classmethod +def install(cls, mh): +super(ADTrustBase, cls).install(mh) +cls.ad = cls.ad_domains[0].ads[0] +cls.install_adtrust() +cls.check_sid_generation() + +# Determine whether the subdomain AD is available +try: +child_ad = cls.host_by_role(cls.optional_extra_roles[0]) +cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:]) +cls.ad_subdomain_hostname = child_ad.hostname +except LookupError: +cls.ad_subdomain = None + +cls.configure_dns_and_time() Please use proper OOP practices when overriding the behavior of the base test class. For starters you could rewrite the install method like this: +@classmethod +def install(cls, mh): +# Determine whether the subdomain AD is available +try: +cls.child_ad = cls.host_by_role(cls.optional_extra_roles[0]) +cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:]) +cls.ad_subdomain_hostname = child_ad.hostname +except LookupError: +raise nose.SkipTest("AFAIK this will skip the whole test class") +super(ADTrustBase, cls).install(mh) with child_ad stored as class attribute, you can override `configure_dns_and_time`: +def configure_dns_and_time(cls): +tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain) # no need to re-implement the function just to get to the child AD DC hostname now +tasks.sync_time(cls.master, cls.child_ad.hostname) You can then use this class as an intermediary in the hierarchy and inherit the other external/non-external trust test classes from it, since most setup method in them are just copy-pasted from this one. -- Martin^3 Babinsky -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust
On 06/30/2016 04:12 PM, Oleg Fayans wrote: Hi Lenka, The changes in test_trust.py are fine. As for tasks.py: 1. I'd rename sync_time_hostname to just sync_time and There's already one sync_time function with different contents, it seemed nicer to create a new function than adding some "switch" into the old one, making it three time longer. 2. I would start ntpd again in the same method: it's no good to keep this thing in mind each time you call it. Leaving ntpd stopped is "legacy" from original function, and as I am not sure which purpose it serves, I decided to leave it as it is. Besides, I would split the changes into 2 patches: one for tasks.py and other for test_trust.py No problem there, + adding ticket info into commit message, as I seem to have forgot again. On 06/30/2016 03:47 PM, Lenka Doudova wrote: Hi, attaching patch with some basic coverage for external trust feature. Bit more detailed info in commit message. Since the feature requires me to run commands previously used only for forest root domains even for subdomains, I made some changes in ipatests/test_integration/tasks.py file, so that it would enable me to reuse existing function without copy-pasting them for one variable change. Lenka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust
Hi Lenka, The changes in test_trust.py are fine. As for tasks.py: 1. I'd rename sync_time_hostname to just sync_time and 2. I would start ntpd again in the same method: it's no good to keep this thing in mind each time you call it. Besides, I would split the changes into 2 patches: one for tasks.py and other for test_trust.py On 06/30/2016 03:47 PM, Lenka Doudova wrote: > Hi, > > attaching patch with some basic coverage for external trust feature. Bit > more detailed info in commit message. > > Since the feature requires me to run commands previously used only for > forest root domains even for subdomains, I made some changes in > ipatests/test_integration/tasks.py file, so that it would enable me to > reuse existing function without copy-pasting them for one variable change. > > > Lenka > > > -- Oleg Fayans Quality Engineer FreeIPA team RedHat. -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust
On 06/30/2016 04:12 PM, Oleg Fayans wrote: Hi Lenka, The changes in test_trust.py are fine. As for tasks.py: 1. I'd rename sync_time_hostname to just sync_time and 2. I would start ntpd again in the same method: it's no good to keep this thing in mind each time you call it. If you start ntpd again you may de-sync AD DC and IPA master again when there is substantial time difference between them (as is usually the case). It is best to left ntpd stopped and even disable it afterwards. Besides, I would split the changes into 2 patches: one for tasks.py and other for test_trust.py On 06/30/2016 03:47 PM, Lenka Doudova wrote: Hi, attaching patch with some basic coverage for external trust feature. Bit more detailed info in commit message. Since the feature requires me to run commands previously used only for forest root domains even for subdomains, I made some changes in ipatests/test_integration/tasks.py file, so that it would enable me to reuse existing function without copy-pasting them for one variable change. Lenka -- Martin^3 Babinsky -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code