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.

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to