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

[Martin Kosek]

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).

Martin