[Freeipa-devel] [PATCH 016, 024, 025] First part of the replica promotion tests + testplan
Martin Basti
mbasti at redhat.com
Tue Mar 1 18:04:51 UTC 2016
On 01.03.2016 14:56, Martin Basti wrote:
>
>
>
> On 01.03.2016 12:37, Martin Basti wrote:
>>
>>
>> On 01.03.2016 12:32, Martin Basti wrote:
>>>
>>>
>>> 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
>> My bad I applied wrong patch
>>
>>>>
>>>>
>>>> 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>
>>>>>
>>>
>>
> I just read the code.
>
> PATCH 16:
> 0)
> PEP8
> ./ipatests/test_integration/test_replica_promotion.py:24:14: E111
> indentation is not a multiple of four
> ./ipatests/test_integration/test_replica_promotion.py:24:14: E113
> unexpected indentation
> ./ipatests/test_integration/test_replica_promotion.py:148:80: E501
> line too long (80 > 79 characters)
> ./ipatests/test_integration/test_replica_promotion.py:150:80: E501
> line too long (81 > 79 characters)
>
> 1)
> workaround is not workaround, because the host entry is removed
> anyway, the error is raised from POST callback, please remove it
> + # Workaround for 5627
> + if "host not found" in result.stderr_text:
> + self.master.run_command(["ipa",
> + "host-del",
> + host.hostname],
> raiseonerr=False)
sorry, I was wrong with this, check is in pre_callback, but please
remove it anyway, I will send patch to fix it ASAP
>
> 2)
> Please name it better, for example "replica" instead of "i"
> + for i in self.replicas:
> + tasks.install_replica(master, i, setup_ca=False,
> + setup_dns=True)
>
> 3)
> Please use constant for domain level (multiple times)
> + result1 = tasks.install_ca(replica1, domain_level=1, raiseonerr=False)
>
> + tasks.install_ca(replica1, domain_level=0)
> + result2 = tasks.install_ca(replica2, domain_level=0,
> raiseonerr=False)
> ... more times
>
> 4)
> This link does not exists, only connect is deprecated not
> ipa-replica-manage at all
> + def test_replica_manage_commands(self):
> + """
> + TestCase:
> http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan
> + #Test_case:_ipa-replica-manage_is_deprecated_in_domain_level_1
> + """
>
> 5)
> Missing testcases:
>
> Test case: Unprivileged users are not allowed to enroll and promote
> clients
> Test case: Replica created using old workflow is functional after
> domain upgrade
> Test case: ipa-csreplica-manage connect is deprecated in domain level 1
> Test case: Replica can be installed using one command
> Test case: Prohibit ipa server uninstallation from disconnecting
> topology segment
>
>
> PATCH 24:
>
> 1)
> why there is this change, how it is related to this patch?:
> def apply_common_fixes(host):
> + prepare_host(host)
> fix_etc_hosts(host)
> fix_hostname(host)
> - prepare_host(host)
>
> 2)
> Why is there this change, how it is related to this patch?:
> def replica_prepare(master, replica):
> - apply_common_fixes(replica)
> fix_apache_semaphores(replica)
> ...
> def install_replica(master, replica, setup_ca=True, setup_dns=False,
> ...
> + apply_common_fixes(replica)
>
> 3)
> why is there this change, how it is related to this patch?:
> -
> + args.extend(['-n', replica.domain.name,
> + '-r', replica.domain.realm])
>
> 4)
> why there force, how is this change related to this patch (domain
> levels)?
> '-w', client.config.admin_password,
> - '--server', master.hostname]
> + '--server', master.hostname,
> + '--force']
> + list(extra_args))
>
> Otherwise domain level related changes LGTM
>
> PATCH 25
>
> LGTM
>
> Martin^2
>
More information about the Freeipa-devel
mailing list