[Freeipa-devel] [PATCH 016, 024, 025] First part of the replica promotion tests + testplan
Oleg Fayans
ofayans at redhat.com
Tue Mar 1 23:12:31 UTC 2016
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
>
>>
>> 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)
>>
>> 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
>>
>> 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.
>>
>> Otherwise domain level related changes LGTM
>>
>> PATCH 25
>>
>> LGTM
>>
>> Martin^2
>>
>
--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0016.9-Integration-tests-for-replica-promotion-feature.patch
Type: text/x-patch
Size: 10939 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160302/607fa675/attachment.bin>
More information about the Freeipa-devel
mailing list