Substituted a hardcoded suffix name with a constant DOMAIN_SUFFIX_NAME
On 12/08/2015 02:33 PM, Oleg Fayans wrote:
> Hi all,
>
>
> The patches are rebased against the current master.
>
> On 12/02/2015 05:10 PM, Martin Basti wrote:
>>
>>
>> On 02.12.2015 16:18, Oleg Fayans wrote:
>>> Hi Martin,
>>>
>>> On 12/01/2015 04:08 PM, Martin Basti wrote:
>>>>
>>>>
>>>> On 27.11.2015 16:26, Oleg Fayans wrote:
>>>>> And patch N 16 passes lint too:
>>>>>
>>>>> On 11/27/2015 04:03 PM, Oleg Fayans wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 11/27/2015 03:26 PM, Martin Basti wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 27.11.2015 15:04, Oleg Fayans wrote:
>>>>>>>> Hi Martin,
>>>>>>>>
>>>>>>>> All your suggestions were taken into account. Both patches are
>>>>>>>> updated. Thank you for your help!
>>>>>>>>
>>>>>>>> On 11/26/2015 10:50 AM, Martin Basti wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 26.11.2015 10:04, Oleg Fayans wrote:
>>>>>>>>>> Hi Martin,
>>>>>>>>>>
>>>>>>>>>> I agree to all your points but one. please, see my comment below
>>>>>>>>>>
>>>>>>>>>> On 11/25/2015 07:42 PM, Martin Basti wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> 0) Note
>>>>>>>>>>> Please be aware of https://fedorahosted.org/freeipa/ticket/5469
>>>>>>>>>>> during
>>>>>>>>>>> KRA testing
>>>>>>>>>>>
>>>>>>>>>>> 1)
>>>>>>>>>>> Please do not use MIN and MAX_DOMAIN_LEVEL constants, this may
>>>>>>>>>>> change
>>>>>>>>>>> over time, use DOMAIN_LEVEL_0 and DOMAIN_LEVEL_1 for domain
>>>>>>>>>>> level 0
>>>>>>>>>>> and 1
>>>>>>>>>>>
>>>>>>>>>>> 2)
>>>>>>>>>>> Why uninstall KRA then server, is not enough just uninstall
>>>>>>>>>>> server
>>>>>>>>>>> which
>>>>>>>>>>> covers KRA uninstall?
>>>>>>>>>>>
>>>>>>>>>>> + def teardown_method(self, method):
>>>>>>>>>>> + for host in self.replicas:
>>>>>>>>>>> + host.run_command(self.kra_uninstall,
>>>>>>>>>>> raiseonerr=False)
>>>>>>>>>>> + tasks.uninstall_master(host)
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> 3)
>>>>>>>>>>> Can be this function more generic? It should allow specify host
>>>>>>>>>>> where
>>>>>>>>>>> KRA should be installed not just master
>>>>>>>>>>>
>>>>>>>>>>> + def test_kra_install_master(self):
>>>>>>>>>>> + self.master.run_command(self.kra_install)
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> 4)
>>>>>>>>>>>
>>>>>>>>>>> TestLevel0(Dummy):
>>>>>>>>>>> Can be the test name more specific, something like
>>>>>>>>>>> TestReplicaPromotionLevel0
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> 5)
>>>>>>>>>>> please remove this, the patch is on review and it will be pushed
>>>>>>>>>>> sooner
>>>>>>>>>>> than tests
>>>>>>>>>>> + @pytest.mark.xfail # Ticket N 5455
>>>>>>>>>>>
>>>>>>>>>>> and as I mentioned in ticket #5455, I cannot reproduce it with
>>>>>>>>>>> ipa-kra-install, so please provide steps to reproduce if you
>>>>>>>>>>> insist
>>>>>>>>>>> that
>>>>>>>>>>> this still does not work as expected with KRA.
>>>>>>>>>>>
>>>>>>>>>>> 6) This is completely wrong, it removes everything that we
>>>>>>>>>>> tried to
>>>>>>>>>>> achieve with previous patches with domain level in CI
>>>>>>>>>>
>>>>>>>>>> Actually, being able to configure domain level per class is WAY
>>>>>>>>>> more
>>>>>>>>>> convenient, than to always have to think which domain level is
>>>>>>>>>> appropriate for which particular test during jenkins job
>>>>>>>>>> configuration. In fact, I should have thought about it from the
>>>>>>>>>> very
>>>>>>>>>> beginning. For example, in test_replica_promotion.py we have on
>>>>>>>>>> class,
>>>>>>>>>> which intiates with domain level = 1, while others - with domain
>>>>>>>>>> level
>>>>>>>>>> 0. With config-based approach, we would have to implement a
>>>>>>>>>> separate
>>>>>>>>>> step that raises domain level. Overall, I am against the approach,
>>>>>>>>>> when you have to remember to set certain domain level in config
>>>>>>>>>> for
>>>>>>>>>> any particular test. The tests themselves should be aware of the
>>>>>>>>>> domain level they need.
>>>>>>>>> I do not say that we should not have something that overrides
>>>>>>>>> settings
>>>>>>>>> in from config in a particular test case, I say your patch is
>>>>>>>>> doing it
>>>>>>>>> wrong.
>>>>>>>>>
>>>>>>>>> I agree it is useful to have param domain_level in install_master,
>>>>>>>>> and
>>>>>>>>> intall_topo methods, but is cannot be MAX_DOMAIN_LEVEL by default,
>>>>>>>>> because with your current patch the domain_level in config is not
>>>>>>>>> used
>>>>>>>>> at all, it will be always MAX_DOMAIN_LEVEL
>>>>>>>>>
>>>>>>>>> For example I want to achieve this goal:
>>>>>>>>> test_vault.py, this test suite can run on domain level1 and on
>>>>>>>>> domain
>>>>>>>>> level0, so with one test we can test 2 domain levels just with
>>>>>>>>> putting
>>>>>>>>> domain level into config file.
>>>>>>>>>
>>>>>>>>> I agree that with extraordinary test like replica promotion test
>>>>>>>>> is, we
>>>>>>>>> need something that allows override the config file.
>>>>>>>>>
>>>>>>>>> As I said bellow, domain_level default value should be None in
>>>>>>>>> install_master and install_topo plugin. If domain level was
>>>>>>>>> specified
>>>>>>>>> use the specified one, if not (value is None) use the domain level
>>>>>>>>> from
>>>>>>>>> config file.
>>>>>>>>
>>>>>>>> Agreed :)
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Martin
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> [PATCH] Enabled setting domain_level per class derived from
>>>>>>>>>>> TestIntegration
>>>>>>>>>>>
>>>>>>>>>>> When I configure domain level 0 in yaml config, how is this
>>>>>>>>>>> supposed to
>>>>>>>>>>> get into install methods when you removed that code?
>>>>>>>>>>>
>>>>>>>>>>> - "--domain-level=%i" % host.config.domain_level
>>>>>>>>>>> + "--domain-level=%i" % domain_level
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> You always use MAX_DOMAIN_LEVEL in this case or whatever is
>>>>>>>>>>> specified in
>>>>>>>>>>> domain_level option.
>>>>>>>>>>> I suggest to use domain_level=None, and when it is None use
>>>>>>>>>>> 'host.config.domain_level', if it is not None, use 'domain_level'
>>>>>>>>>>>
>>>>>>>>>>> With this we can specify domain level in config file for test
>>>>>>>>>>> that
>>>>>>>>>>> can
>>>>>>>>>>> be used for both domain levels and you can manually specify
>>>>>>>>>>> domain
>>>>>>>>>>> level
>>>>>>>>>>> for test that requires specific domain level.
>>>>>>>>>>>
>>>>>>>>>>> Also this should go away
>>>>>>>>>>>
>>>>>>>>>>> @classmethod
>>>>>>>>>>> def install(cls, mh):
>>>>>>>>>>> + if hasattr(cls, "domain_level") and cls.master:
>>>>>>>>>>> + cls.master.config.domain_level = cls.domain_level
>>>>>>>>>>> if cls.topology is None:
>>>>>>>>>>> return
>>>>>>>>>>>
>>>>>>>>>>> I do not see reason why test should override configuration in
>>>>>>>>>>> config in
>>>>>>>>>>> this case.
>>>>>>>>>>>
>>>>>>>>>>> Martin
>>>>>>>>>>>
>>>>>>>>>>> On 25.11.2015 16:44, Oleg Fayans wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> Here is the updated version of the patch (more tests + fixed the
>>>>>>>>>>>> issues of the first one) + patch 0017, that implements the
>>>>>>>>>>>> necessary
>>>>>>>>>>>> changes in the background code, i. e. patch 16 does not work
>>>>>>>>>>>> without
>>>>>>>>>>>> patch 17
>>>>>>>>>>>>
>>>>>>>>>>>> On 11/18/2015 05:20 PM, Martin Basti wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 09.11.2015 15:09, Oleg Fayans wrote:
>>>>>>>>>>>>>> Hi guys,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Here are first two automated testcases from this (so far
>>>>>>>>>>>>>> incomplete)
>>>>>>>>>>>>>> testplan:
>>>>>>>>>>>>>> http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Testplan review is highly appreciated
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>> PATCH 16: NACK
>>>>>>>>>>>>>
>>>>>>>>>>>>> 1)
>>>>>>>>>>>>> What is the reason to add an unused parameter to
>>>>>>>>>>>>> 'domain_level' to
>>>>>>>>>>>>> install_topo()?
>>>>>>>>>>>>> Also it is good practise to add new option as the last
>>>>>>>>>>>>> parameter.
>>>>>>>>>>>>>
>>>>>>>>>>>>> 2)
>>>>>>>>>>>>> cab you in both tests specify a domain level with constant
>>>>>>>>>>>>> instead of
>>>>>>>>>>>>> number literal?
>>>>>>>>>>>>>
>>>>>>>>>>>>> 3)
>>>>>>>>>>>>> both test call install_topo with custom domain level, but it
>>>>>>>>>>>>> cannot
>>>>>>>>>>>>> work
>>>>>>>>>>>>> because 1) (did you run the test?)
>>>>>>>>>>>>>
>>>>>>>>>>>>> 4)
>>>>>>>>>>>>> How the test "TestLevel1" is supposed to work?
>>>>>>>>>>>>> Respectively why there is call of install_topo() that installs
>>>>>>>>>>>>> replica.
>>>>>>>>>>>>> As this test just tests that ipa-replica-prepare is not working
>>>>>>>>>>>>> anymore,
>>>>>>>>>>>>> is it worth to spend 20 minutes with installing replica and
>>>>>>>>>>>>> then
>>>>>>>>>>>>> just no
>>>>>>>>>>>>> tot use it? IMO to install master in install step is enough.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Martin^2
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>> ./make-lint
>>>>>>> ************* Module ipatests.test_integration.base
>>>>>>> ipatests/test_integration/base.py:66: [E1101(no-member),
>>>>>>> IntegrationTest.install] Class 'IntegrationTest' has no
>>>>>>> 'domain_level'
>>>>>>> member)
>>>>>>> ************* Module ipatests.test_integration.test_replica_promotion
>>>>>>> ipatests/test_integration/test_replica_promotion.py:16:
>>>>>>> [E1101(no-member), Dummy.install] Class 'Dummy' has no 'domain_level'
>>>>>>> member)
>>>>>>> ipatests/test_integration/test_replica_promotion.py:117:
>>>>>>> [E1101(no-member),
>>>>>>> TestCAInstall.test_ca_install_without_replica_file]
>>>>>>> Module 'ipatests.test_integration.tasks' has no 'setup_replica'
>>>>>>> member)
>>>>>>>
>>>>>>>
>>>>>>> Is it so hard to run pylint before patch is posted for review?
>>>>>>
>>>>>> Sorry, my bad. Fixed.
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>> 1)
>>>> Why is this change in the patch?
>>>> - # Clean up the test directory
>>>> - host.run_command(['rm', '-rvf', host.config.test_dir])
>>>
>>> Otherwise 2 out of 8 tests fail with IOError at line 78 of
>>> ipatests/test_integration/tasks.py
>>>
>>> I do not understand yet how does this happen, but if you remove
>>> ipatests folder once, it then fails to be created again.
>>>
>> So this should be in separated patch and investigated properly.
> Agree. Removed
>>
>>>> -
>>>>
>>>>
>>>> 2)
>>>> is enough to have this check only in install_master, install_topo can
>>>> pass None to install_master
>>>> + if domain_level is None:
>>>> + domain_level = master.config.domain_level
>>>
>>> Done
>>>
>>>>
>>>>
>>>> 3)
>>>> IMO replica-manage del should cleanup hosts entry, so following code
>>>> should not be needed.
>>>> + if cleanhost:
>>>> + kinit_admin(master)
>>>> + master.run_command(["ipa", "host-del", "--updatedns",
>>>> replica.hostname],
>>>> + raiseonerr=False)
>>>
>>> Well, in fact it does not. At least the corresponding dns record stays
>>> and causes the subsequent ipa-client-install to fail. Probably it's a
>>> bug. On the other hand, if I promote an existing client to replica and
>>> then delete this replica, then, I probably want the host record (that
>>> was created during client-install) to stay in the system. So, does not
>>> look like a bug to me.
>> No you don't, because replica uninstallation also removes the client.
>>
>> I tried it with ipa42, ipa-replica-manage del removes host entry, and
>> DNS A records, only SSHFP records stay there (I'm not sure if it is bug
>> or feature)
>>
>> Also I received this message
>> """
>> Failed to cleanup replica1.ipa.test DNS entries: no matching entry found
>> You may need to manually remove them from the tree
>> """
>> But, A record has been removed, so this is probably false positive and
>> it needs to have a ticket.
>
> Agree, that was an issue with my setup.
> Removed
>
>>>
>>>>
>>>> 4)
>>>> This variable is not used
>>>> + kra_uninstall = ["ipa-kra-install", "--uninstall", "-U"]
>>>
>>> Removed
>>>
>>>>
>>>> 5)
>>>> Why do you need this
>>>> + kra_install = ["ipa-kra-install", "-U", "-p",
>>>> config.dirman_password]
>>>> when you implemented tasks.install_kra that returns the exactly the same
>>>> result?
>>>
>>> Right. Removed
>>>
>>>>
>>>> 6)
>>>> PEP8
>>>> ./ipatests/test_integration/tasks.py:928:1: E302 expected 2 blank lines,
>>>> found 1
>>>> ./ipatests/test_integration/tasks.py:934:1: E302 expected 2 blank lines,
>>>> found 1
>>>> ./ipatests/test_integration/tasks.py:939:1: E302 expected 2 blank lines,
>>>> found 1
>>>> ./ipatests/test_integration/tasks.py:943:1: E302 expected 2 blank lines,
>>>> found 1
>>>> ./ipatests/test_integration/tasks.py:950:80: E501 line too long (80 > 79
>>>> characters)
>>>>
>>>> ./ipatests/test_integration/test_replica_promotion.py:29:80: E501 line
>>>> too long (85 > 79 characters)
>>>> ./ipatests/test_integration/test_replica_promotion.py:64:80: E501 line
>>>> too long (85 > 79 characters)
>>>> ./ipatests/test_integration/test_replica_promotion.py:67:80: E501 line
>>>> too long (88 > 79 characters)
>>>> ./ipatests/test_integration/test_replica_promotion.py:93:80: E501 line
>>>> too long (80 > 79 characters)
>>>> ./ipatests/test_integration/test_replica_promotion.py:94:80: E501 line
>>>> too long (83 > 79 characters)
>>>> ./ipatests/test_integration/test_replica_promotion.py:118:80: E501 line
>>>> too long (81 > 79 characters)
>>>> ./ipatests/test_integration/test_replica_promotion.py:128:80: E501 line
>>>> too long (80 > 79 characters)
>>>> ./ipatests/test_integration/test_replica_promotion.py:129:80: E501 line
>>>> too long (82 > 79 characters)
>>>> ./ipatests/test_integration/test_replica_promotion.py:181:80: E501 line
>>>> too long (80 > 79 characters)
>>>
>>> Most of these complaints are unrelated to the current patches.
>>> It's better to create a separate patch addressing PEP8 errors.
>>>
>> I beg for your pardon, those PEP8 errors have been introduced by your
>> patches.
>
> Fixed
>
>>>>
>>>> 7)
>>>> Why this must be stored in instance? IMO to have it stored as local
>>>> variable is perfect
>>>> TestKRAInstall, TestCAInstall
>>>> self.replica1_filename = tasks.get_replica_filename(replica1)
>>>> self.replica2_filename = tasks.get_replica_filename(replica2)
>>>
>>> Agree. Fixed
>>>
>>
>
>
>
--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From 761c3e4da49b2686f6e39c79863a129505634cc0 Mon Sep 17 00:00:00 2001
From: Oleg Fayans <ofay...@redhat.com>
Date: Tue, 8 Dec 2015 23:44:46 +0100
Subject: [PATCH] Prepared integration tests for replica promotion
---
ipatests/test_integration/test_replica_promotion.py | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/ipatests/test_integration/test_replica_promotion.py b/ipatests/test_integration/test_replica_promotion.py
index 9282a47aa07682a5aec296c941f5ebbff84ab89c..48d97d62cca070a1d3d46499767ad5c295d00425 100644
--- a/ipatests/test_integration/test_replica_promotion.py
+++ b/ipatests/test_integration/test_replica_promotion.py
@@ -4,6 +4,7 @@ from ipatests.test_integration.test_caless import assert_error
from ipalib.constants import DOMAIN_LEVEL_0
from ipalib.constants import DOMAIN_LEVEL_1
from env_config import get_global_config
+from ipalib.constants import DOMAIN_SUFFIX_NAME
class Dummy(IntegrationTest):
@@ -61,7 +62,7 @@ class TestReplicaPromotionLevel0(Dummy):
http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan#Test_case:
_ipa-restore_after_domainlevel_raise_restores_original_domain_level
"""
- command = ["ipa", "topologysegment-find", "realm"]
+ command = ["ipa", "topologysegment-find", DOMAIN_SUFFIX_NAME]
tasks.install_replica(self.master, self.replicas[0])
backup_file = tasks.ipa_backup(self.master)
self.master.run_command(["ipa", "domainlevel-set", "1"])
--
2.4.3
--
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
