[Freeipa-devel] [PATCHES 202-222] Ipaplatform refactoring

[Petr Viktorin]

On 06/25/2014 07:16 PM, Tomas Babej wrote:
>
> On 06/25/2014 04:59 PM, Tomas Babej wrote:
>>
>> On 06/25/2014 04:13 PM, Tomas Babej wrote:
>>>
>>> On 06/25/2014 04:01 PM, Tomas Babej wrote:
>>>>
>>>> On 06/25/2014 10:48 AM, Petr Viktorin wrote:
>>>>> On 06/19/2014 03:52 PM, Tomas Babej wrote:
>>>>>>
>>>>>> On 06/19/2014 12:52 PM, Tomas Babej wrote:
>>>>>>> On 06/18/2014 10:52 AM, Petr Viktorin wrote:
>>>>>>>> On 06/17/2014 02:15 PM, Tomas Babej wrote:
>>>>>>>>> On 06/17/2014 12:03 PM, Timo Aaltonen wrote:
>>>>>>>>>> On 17.06.2014 11:16, Martin Kosek wrote:
>>>>>>>>> Attached is a new version of patch 226, and a new patch 228,
>>>>>>>>> which moves
>>>>>>>>> the paths from installers to the paths module.
>>>>>>>> In patch 226, there's another "certificated" typo in
>>>>>>>> remove_ca_cert_from_systemwide_ca_store
>>>>>>>>
>>>>>>>>> I greped the repository, and I do not see many paths lurking
>>>>>>>>> around any
>>>>>>>>> more, there are only some in the error messages (as these can't be
>>>>>>>>> reliably replaced automatically, and will need some manual love).
>>>>>>>>>
>>>>>>>>> If you see any forgotten paths, which should be added to the
>>>>>>>>> module, let
>>>>>>>>> me know.
>>>>>
>>>>> Well, since you asked...
>>>>>
>>>>> install/tools/ipa-upgradeconfig:236:
>>>>> ipautil.run([paths.PKI_SETUP_PROXY, '-pki_instance_root=/var/lib'
>>>>> ipaserver/install/cainstance.py:1330: "-pki_instance_root=/var/lib",
>>>>>
>>>>> ipaserver/install/dsinstance.py:209:InstallLdifFile=
>>>>> /var/lib/dirsrv/boot.ldif
>>>>> ipaserver/install/dsinstance.py:210:inst_dir=
>>>>> /var/lib/dirsrv/scripts-$SERVERID
>>>>>
>>>>> ipaserver/install/ipa_backup.py:464: '--exclude=/var/lib/ipa/backup',
>>>>>
>>>>> ipatests/test_integration/tasks.py:451: host.run_command("find
>>>>> /var/lib/sss/db -name '*.ldb' | "
>>>>>
>>>>> install/tools/ipa-replica-conncheck:403:
>>>>> "/usr/sbin/ipa-replica-conncheck " +
>>>>> install/tools/ipa-replica-conncheck:414:
>>>>> print_info("/usr/sbin/ipa-replica-conncheck " + "
>>>>> ".join(remote_check_opts))
>>>>>
>>>>> ipapython/ipautil.py:296:        env["PATH"] =
>>>>> "/bin:/sbin:/usr/kerberos/bin:/usr/kerberos/sbin:/usr/bin:/usr/sbin"
>>>>>
>>>>> ipaserver/install/cainstance.py:88:ConfigFile =
>>>>> /usr/share/pki/ca/conf/database.ldif
>>>>>
>>>>> ipaserver/install/bindinstance.py:829:
>>>>> ipautil.run(['/usr/libexec/generate-rndc-key.sh'])
>>>>>
>>>>
>>>> /me will think twice about teasing nex time.
>>>>
>>>> This are paths requiring manual changes in one way or the other and
>>>> as such cannot be handled by my tool. Let's not stall the patcheset
>>>> on this. We can fix these (and surely there are other) as we go along.

OK, not a reason to hold the patch back.
But, the fact that the tool can't handle them doesn't make them less 
important. Let's keep the ticket open, or open a new one.

>>>>> I guess it'll be a while before we catch them all, but now it's at
>>>>> least clear where these paths should be, so anyone porting to
>>>>> another distro can send patches (or tickets) upstream.
>>>>>
>>>>>>>> I see another duplicate:
>>>>>>>>      SSS_KRB5_INCLUDE_D = "/var/lib/sss/pubconf/krb5.include.d"
>>>>>>>>      SSSD_PUBCONF_KRB5_INCLUDE_D_DIR =
>>>>>>>> "/var/lib/sss/pubconf/krb5.include.d/"
>>>>>
>>>>> Could you just pick one instead? Would ipa_backup.py break if it
>>>>> had a trailing slash here?
>>>>>
>>>>
>>>> Yes. I verified it produces the same result with or without trailing
>>>> slash, fixed.
>>>>
>>>>
>>>>> In ipa-client-install, if you set:
>>>>>     NSSWITCH_CONF = paths.NSSWITCH_CONF
>>>>> then you should only use one of those later. (Preferably paths.*,
>>>>> to get rid of the redundant constants.)
>>>>> Perhaps this is for another patch that would clean up all the cases
>>>>> where these trivial module variables are used.
>>>>>
>>>>
>>>> I agree. Fixed this occurence.

I've opened an easyfix ticket for the rest:
https://fedorahosted.org/freeipa/ticket/4399

>>>>>>> Fixed all mentioned issues. I also attached a patch 230, which
>>>>>>> removes
>>>>>>> the base Authconfig class.
>>>>>
>>>>>
>>>>>> Attaching one additional patch, which removes unnecessary build
>>>>>> warnings.
>>>>>>
>>>>>
>>>>> 226, 230, 231 look good
>>>>>
>>>>
>>>> Attaching whole updated patchset.
>>>
>>> Attaching one more patch which should fix broken CI tests.
>>>
>>
>> Self-NACK - It seems I omitted one occurence of NSSWITCH_CONF in
>> ipa-client-install, fixed now.
>>
>> Attaching the whole patchset for your convenience.
>> --
> Attaching a correct patchset this time.

I hate to break it to you, but...
             you sent the wrong patch 228 :(


They're pretty independent, and there are fixes for failing tests, so I 
went ahead with the other ones.

226, 230, 231, 235: ACK, pushed to master: 
c8511d3b3baa389069156bf9991a9f4c7d64cf4a

-- 
Petr³