[Freeipa-devel] [PATCH 016 - 017] First part of the replica promotion tests + testplan
Martin Basti
mbasti at redhat.com
Tue Mar 1 11:32:46 UTC 2016
On 29.02.2016 13:16, Oleg Fayans wrote:
> Hi all,
>
> Finally the tests pass.
>
> The patch 0024 applies on top of patch 0022 (please, consider reviewing
> it also). Besides, the whole functionality depends on Martin's patch N 0421
>
> All patches pass pylint.
hello,
I cannot apply patches on master branch
Martin^2
>
>
> On 12/19/2015 11:56 PM, Martin Basti wrote:
>>
>> On 17.12.2015 10:04, Oleg Fayans wrote:
>>> Hi Martin,
>>>
>>> I am sorry, in my previous email I attached the old version of patch
>>> 0016. The correct on is attached.
>>>
>>> On 12/16/2015 05:47 PM, Martin Basti wrote:
>>>> On 16.12.2015 15:39, Martin Basti wrote:
>>>>> On 15.12.2015 10:29, Oleg Fayans wrote:
>>>>>> Hi Martin,
>>>>>>
>>>>>> The updated patches are attached. Patch 0017 includes all changes from
>>>>>> patch 0018, so, if you approve this one, there would be no need to
>>>>>> continue with the review of 0018. This one contains all changes
>>>>>> related
>>>>>> to you remarks from 0018 review. Please see my explanation on the
>>>>>> stdout+stderr part in the thread from patch 0018.
>>>>>> With these two patches applied one of the tests fails due this bug:
>>>>>> https://fedorahosted.org/freeipa/ticket/5550
>>>>>>
>>>>>> On 12/09/2015 12:17 PM, Martin Basti wrote:
>>>>>>> On 09.12.2015 12:10, Martin Basti wrote:
>>>>>>>> On 09.12.2015 11:14, Oleg Fayans wrote:
>>>>>>>>> Hi Martin
>>>>>>>>>
>>>>>>>>> On 12/09/2015 10:30 AM, Martin Basti wrote:
>>>>>>>>>> On 08.12.2015 23:48, Oleg Fayans wrote:
>>>>>>>>>>> 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
>>>>>>>>>>>>>>
>>>>>>>>>> This patch is missing something.
>>>>>>>>> I am sorry, I forgot to revert my previous change. The correct
>>>>>>>>> patch is
>>>>>>>>> attached
>>>>>>>>>
>>>>>>>> ************* Module
>>>>>>>> ipatests.test_integration.test_replica_promotion
>>>>>>>> ipatests/test_integration/test_replica_promotion.py:15:
>>>>>>>> [E1123(unexpected-keyword-arg), Dummy.install] Unexpected keyword
>>>>>>>> argument 'domain_level' in function call)
>>>>>>>> ipatests/test_integration/test_replica_promotion.py:15:
>>>>>>>> [E1101(no-member), Dummy.install] Class 'Dummy' has no
>>>>>>>> 'domain_level'
>>>>>>>> member)
>>>>>>>> ipatests/test_integration/test_replica_promotion.py:19:
>>>>>>>> [E1101(no-member), Dummy.teardown_method] Module
>>>>>>>> 'ipatests.test_integration.tasks' has no 'uninstall_replica' member)
>>>>>>>> ipatests/test_integration/test_replica_promotion.py:67:
>>>>>>>> [E1101(no-member), TestReplicaPromotionLevel0.test_backup_restore]
>>>>>>>> Module 'ipatests.test_integration.tasks' has no 'ipa_backup' member)
>>>>>>>> ipatests/test_integration/test_replica_promotion.py:72:
>>>>>>>> [E1101(no-member), TestReplicaPromotionLevel0.test_backup_restore]
>>>>>>>> Module 'ipatests.test_integration.tasks' has no 'ipa_restore'
>>>>>>>> member)
>>>>>>>> ipatests/test_integration/test_replica_promotion.py:120:
>>>>>>>> [E1123(unexpected-keyword-arg), TestCAInstall.install] Unexpected
>>>>>>>> keyword argument 'domain_level' in function call)
>>>>>>>>
>>>>>>> Sorry I forgot to apply patch 17, my bad, I'm continuing with review
>>>>> LGTM, I haven't had time to test it, but if you are sure that test is
>>>>> working, we may push this.
>>>>>
>>>> Is this expected due the bug you mentioned?
>>>> _____
>>>> __________________________________________________________________________
>>>>
>>>> TestReplicaPromotionLevel0.test_kra_install_master
>>>> ________________________________________________________________________________
>>>>
>>>>
>>>>
>>>> self =
>>>> <ipatests.test_integration.test_replica_promotion.TestReplicaPromotionLevel0
>>>>
>>>> object at 0x7f5071a59e50>
>>>>
>>>> def test_kra_install_master(self):
>>>> result1 = tasks.install_kra(self.master, raiseonerr=False)
>>>>> assert result1.returncode == 0, result1.stderr_text
>>>> E AssertionError: Usage: ipa-kra-install [options] [replica_file]
>>>> E
>>>> E ipa-kra-install: error: Replica file
>>>> /root/ipatests/replica-info.gpg does not exist
>>>> E The ipa-kra-install command failed. See
>>>> /var/log/ipaserver-kra-install.log for more information
>>>> E
>>>> E assert 2 == 0
>>>> E + where 2 = <pytest_multihost.transport.SSHCommand object at
>>>> 0x7f5071adbd50>.returncode
>>>>
>> IMO the test needs fix, KRA on replica file needs KRA related
>> certificates in replica file
>>
>> [ipa.ipatests.test_integration.host.Host.replica2.ParamikoTransport] RUN
>> ['ipa-kra-install', '-U', '-p', 'Secret123',
>> '/root/ipatests/replica-info.gpg']
>> [ipa.ipatests.test_integration.host.Host.replica2.cmd27] RUN
>> ['ipa-kra-install', '-U', '-p', 'Secret123',
>> '/root/ipatests/replica-info.gpg']
>> [ipa.ipatests.test_integration.host.Host.replica2.cmd27] Missing KRA
>> certificates, please create a new replica file.
>> [ipa.ipatests.test_integration.host.Host.replica2.cmd27] The
>> ipa-kra-install command failed. See /var/log/ipaserver-kra-install.log
>> for more information
>> [ipa.ipatests.test_integration.host.Host.replica2.cmd27] Exit code: 1
>> FAILED
>> traceback
>>
>> self = <ipatests.test_integration.test_replica_promotion.TestKRAInstall
>> object at 0x7f660bc1a590>
>>
More information about the Freeipa-devel
mailing list