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

[Petr Spacek]

On 17.6.2014 14:50, Tomas Babej wrote:
>
> On 06/17/2014 02:44 PM, Petr Spacek wrote:
>> On 17.6.2014 14:15, Tomas Babej wrote:
>>>
>>> On 06/17/2014 12:03 PM, Timo Aaltonen wrote:
>>>> On 17.06.2014 11:16, Martin Kosek wrote:
>>>>> On 06/16/2014 07:50 PM, Petr Viktorin wrote:
>>>>>> On 06/16/2014 02:53 PM, Tomas Babej wrote:
>>>>>>> On 06/10/2014 05:07 PM, Petr Viktorin wrote:
>>>>>>>> On 06/10/2014 10:13 AM, Tomas Babej wrote:
>>>>>>>>> On 06/06/2014 01:04 PM, Petr Viktorin wrote:
>>>>>>>>>> On 06/05/2014 03:14 PM, Petr Viktorin wrote:
>>>>>>>>>>> On 06/04/2014 11:42 AM, Tomas Babej wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> the following set of patches implements the ticket:
>>>>>>>>>>>>
>>>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/4052
>>>>>>>>>>>>
>>>>>>>> [...]
>>>>>> 0202: OK
>>>>>> 0203: OK
>>>>>> 0204: OK
>>>>>> 0205: OK
>>>>>> 0206: OK
>>>>>> 0207: OK
>>>>>> (sorry for the conflict!)
>>>>>>
>>>>>> 0208: OK
>>>>>> 0209: OK
>>>>>> 0210: OK
>>>>>> 0211: OK
>>>>>> 0212: OK
>>>>>> 0213: OK
>>>>>> 0214: OK
>>>>>> 0215: OK
>>>>>> 0216: OK
>>>>>> 0217: OK
>>>>>> 0218: OK
>>>>>> 0219: OK
>>>>>> 0220: OK
>>>>>> 0221: OK
>>>>>> 0222: OK
>>>>>>
>>>>>> modify_nsswitch_pam_stack and modify_pam_to_use_krb5 are missing
>>>>>> the `self`
>>>>>> argument.
>>>>>>
>>>>>> Rebasing this all the time must be painful, so to avoid another
>>>>>> review
>>>>>> round-trip I've had Tomáš ACK the attached four-liner on IRC.
>>>>> Thanks guys!
>>>>>
>>>>> I looked at the changes and have couple questions:
>>>>>
>>>>> 1) What is the motivation for keeping AuthConfig infrastructure
>>>>> around? I
>>>>> thought it is replaced by the tasks concept that are easier to
>>>>> customize in
>>>>> other platforms. IMO, it just obfuscates the process.
>>>>>
>>>>> How is
>>>>> def modify_pam_to_use_krb5(self, statestore):
>>>>> auth_config = FedoraAuthConfig()
>>>>> statestore.backup_state('authconfig', 'krb5', True)
>>>>> auth_config.enable("krb5")
>>>>> auth_config.add_option("nostart")
>>>>> auth_config.execute()
>>>>> more readable than
>>>>> def modify_pam_to_use_krb5(self, statestore):
>>>>> statestore.backup_state('authconfig', 'krb5', True)
>>>>> ipautil.run("authconfig --enablekrb5 --nostart")
>>>>> ? And this was just the easy example. Also, documentation in
>>>>> AuthConfig base
>>>>> class refers to nonexistent paths.
>>>>>
>>>>> 2) There are still many non-converted paths in ipa-client installers:
>>>>>
>>>>> $ git grep "bin/" ipa-client/
>>>>> ...
>>>>> ipa-client/ipa-install/ipa-client-install:SSH_AUTHORIZEDKEYSCOMMAND =
>>>>> '/usr/bin/sss_ssh_authorizedkeys'
>>>>> ipa-client/ipa-install/ipa-client-install:SSH_PROXYCOMMAND =
>>>>> '/usr/bin/sss_ssh_knownhostsproxy'
>>>>> ipa-client/ipa-install/ipa-client-install: (sout, serr, returncode) =
>>>>> run(["/usr/bin/certutil", "-L", "-d", "/etc/pki/nssdb", "-n",
>>>>> nickname],
>>>>> raiseonerr=False)
>>>>> ipa-client/ipa-install/ipa-client-install: run(["/usr/bin/certutil",
>>>>> "-D", "-d", "/etc/pki/nssdb", "-n", "IPA CA"])
>>>>> ipa-client/ipa-install/ipa-client-install: run(["/usr/bin/certutil",
>>>>> "-D", "-d", "/etc/pki/nssdb", "-n", client_nss_nickname])
>>>>> ...
>>>>>
>>>>> We should convert at least those as ipa-client will be the most
>>>>> platformized
>>>>> module (more than the server, IMO).
>>>> and many others all over the place, just 'git grep /etc/'
>>>>
>>>> working on the debian client patches now..
>>>>
>>>>
>>>
>>> Attached is a new version of patch 226, and a new patch 228, which moves
>>> the paths from installers to the paths module.
>>>
>>> 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).
>> I don't know the code but why it is not possible to use
>> "%s" % (paths.blahblah) ?
>>
>> IMHO paths should never be hardcoded in strings/messages shown to user.
>>
>> Petr^2 Spacek
>
> Of course they can, they just cannot be replaced by the tool I developed
> for this refactoring, and need to be replaced manually, as the tool
> lacks the capability.
>
> They need to be replaced manually.

Ah, sure. I didn't notice you were speaking about automatic replacement. I'm 
sorry!

Petr^2 Spacek

>
>>
>>> If you see any forgotten paths, which should be added to the module, let
>>> me know.