[Freeipa-devel] [PATCH 016, 024, 025] First part of the replica promotion tests + testplan
Martin Basti
mbasti at redhat.com
Thu Mar 3 17:38:33 UTC 2016
On 02.03.2016 13:47, Oleg Fayans wrote:
> Hi Martin,
>
> I've made the requested changes.
>
> The full set of necessary patches is attached.
>
>
> On 03/02/2016 10:05 AM, Martin Basti wrote:
>>
>> 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.
> Done, patch 0027
>
>>>>> 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
> Removed
>
>>>>> 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
> Done. Patch 0026
>
>>>
>>>>> 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
> Done. Patch 0026
>
>>>>> 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
> I've run the tests without this option again at it passed. Must have
> been some temporary issue. Removed this change.
>
>>>>> Otherwise domain level related changes LGTM
>>>>>
>>>>> PATCH 25
>>>>>
>>>>> LGTM
>>>>>
>>>>> Martin^2
>>>>>
1)
this method is unused please remove it
def test_kra_install_master(self):
2)
Why are these there? I do not see any usage
from env_config import get_global_config
config = get_global_config()
3) nitpick
+ num_clients = 0
this is set by default
otherwise LGTM
Results of testing tomorrow.
Martin^2
More information about the Freeipa-devel
mailing list