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

[Petr Viktorin]

On 06/25/2014 09:48 PM, Tomas Babej wrote:
>
> On 06/25/2014 09:35 PM, Petr Viktorin wrote:
>> 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 :(
>>
>
> No way! That's a official fail combo on my part. Here's the correct patch.

ACK, pushed to master: e5e42fc83ae74f0e0c68e68417a39fe6f2f2ae63





-- 
Petr³