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 > [email protected] > https://www.redhat.com/mailman/listinfo/freeipa-devel -- Tomas Babej Associate Software Engineer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org _______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
