[Freeipa-devel] [PATCH 016 - 017] First part of the replica promotion tests + testplan
Martin Basti
mbasti at redhat.com
Wed Dec 16 16:47:53 UTC 2015
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
More information about the Freeipa-devel
mailing list