On 17.06.2014 15: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). > > If you see any forgotten paths, which should be added to the module, let > me know.
Sure thing! Looks more complete now, and at least the paths I was patching before (in ipa-client-automount) are fixed. -- t _______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
