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

[Tomas Babej]

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.

>
>> If you see any forgotten paths, which should be added to the module, let
>> me know.
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel

-- 
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org