[Freeipa-devel] [PATCH 016, 024, 025] First part of the replica promotion tests + testplan

Martin Basti mbasti at redhat.com
Wed Mar 2 09:05:57 UTC 2016 


On 02.03.2016 00:12, Oleg Fayans wrote:
> Hi Martin,
>
> On 03/01/2016 07:04 PM, Martin Basti wrote:
>>
>> 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
> Done
I realized that the fix I'm working on is for 4.4 only, so for 4.3 add 
this as separated patch.
>>> 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)
> Done
>
>>> 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
> Done
>
>>> 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
>>> +        """
> Fixed
>
>>> 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
>>>
> They are on the way, not fully ready yet
>
>>> 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)
> Good catch! That was one of my attempts to address the issue that was
> successfully resolved in patch 0025. Will remove it once we agree on the
> rest of the changes
>
>>> 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)
> Just to make this call independent from domain level (at domain_level 1
> replica_prepare never gets called)
It should be in separate commit, because it is not related to adding 
domain_level in class functionality
>
>
>>> 3)
>>> why is there this change, how it is related to this patch?:
>>> -
>>> +        args.extend(['-n', replica.domain.name,
>>> +                     '-r', replica.domain.realm])
> At least -r is a required parameter. -n was added for further
> robustness. Can be safely removed, though
It should be in separate commit, as this is not related to domain levels
>
>>> 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))
> client refuses to install unless everything is super clear in the dns
> setup (including reverse zone). Otherwise the installer fails and
> informs you that you may use '--force' at your own risk. I can rerun the
> tests without this option to provide you with the exact output, if you like.
It should be in separated commit, because it is not related to domain levels
>
>>> Otherwise domain level related changes LGTM
>>>
>>> PATCH 25
>>>
>>> LGTM
>>>
>>> Martin^2
>>>

More information about the Freeipa-devel mailing list