On 03/11/14 10:28, David Kupka wrote:
On 10/30/2014 10:42 AM, Martin Basti wrote:
On 29/10/14 17:23, David Kupka wrote:
On 10/29/2014 02:34 PM, David Kupka wrote:
On 10/24/2014 03:05 PM, David Kupka wrote:
On 10/24/2014 01:06 PM, David Kupka wrote:
On 10/24/2014 10:43 AM, Martin Basti wrote:
On 24/10/14 09:51, David Kupka wrote:
https://fedorahosted.org/freeipa/ticket/4585
NACK

1)
Why is there line with 'DS System User?' The comment should depend on
service.

+            args = [
+                paths.USERADD,
+                '-g', group,
+                '-c', 'DS System User',
+                '-d', homedir,
+                '-s', shell,
+                '-M', '-r', name,
+            ]

This was part of the original code and I didn't notice it. Nice catch,
thanks.


2)
code create_system_user is duplicated between base and redhat tasks
with
platform dependent changes.
IMO it would be better to have one method to create user, with
keyword
arguments.  And then platform dependent method which will call
method to
create user with appropriate arguments (or with default arguments)


You're right it was ugly.



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

I shouldn't break SOLID principles.



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

Using super is probably better that explicit naming of parent class.
Let user (developer) override UID/GID and hope that he knows why ...


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




In your former patch you had pki homedir path VAR_LIB_PKI_DIR :

+        if name == 'pkiuser':
+            uid = 17
+            gid = 17
+            homedir = paths.VAR_LIB_PKI_DIR
+            shell = paths.NOLOGIN
+            comment = 'CA System User'

in last patch you change it back to:

          homedir=paths.VAR_LIB,

so what is the correct path?


The setup package (soft static allocation) claims that pkiuser should use '/usr/share/pki' as home directory. Since pkiuser has /sbin/nologin set as a login shell it's unable to login and does't need home directory at all. We could use '--system' option of useradd utility to skip home directory creation or change to proposed value or just leave the old value and all will result in no change in behavior. I'm not sure if the '--system' option is available universally. IIRC it used to be Red Hat-like-systems specific extension.


If there is no reason to change homedir, don't do it.
I will continue with reviewing then.
Martin^2

--
Martin Basti

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

Reply via email to