Re: [Freeipa-devel] [PATCH] 0043 fix ipa-dns-install to not require DM password
On Fri, 2011-01-07 at 10:44 +0100, Jan Zelený wrote: > Simo Sorce wrote: > > On Fri, 2011-01-07 at 09:53 +0100, Jan Zelený wrote: > > > Simo Sorce wrote: > > > > On Thu, 2011-01-06 at 10:35 +0100, Jan Zelený wrote: > > > > > Simo Sorce wrote: > > > > > > This patch makes it possible to run ipa-dns-install and use the > > > > > > admin kerberos credentials. > > > > > > > > > > > > Fixes #686. > > > > > > > > > > > > Simo. > > > > > > > > > > Nack, I have some comments: > > > > > > > > > > Exception handling (chunk #4): > > > > > Those prints should go away. But the main thing: that particular part > > > > > of code doesn't seem to produce any exceptions, which should be > > > > > handled > > > > > > > > > > Function ldap_disconnect isn't used anywhere. That makes me wonder - > > > > > is it redundant or should it be somewhere in the code. I guess this > > > > > is a policy issue - either we want the connection to stay as long as > > > > > possible or we want to use it only for a certain set of commands and > > > > > then disconnect it. > > > > > > > > Attached new patch that fixes hunk #4. > > > > Actually I ended up using ldap_disconnect() here as we need to test the > > > > ldap connection anyway. > > > > I also had to do minor changes to Bindinstance() as the code was > > > > clearing self.fqdn after Service.__init__ set it. > > > > > > > > Simo. > > > > > > Nack, > > > IMO in case invalid credentials are given and the script runs in > > > unattended mode, the script should exit and not ask for the password. > > > > Good catch! > > > > New patch attached. > > > > Simo. > > ack > > Jan Thanks, pushed to master. Simo. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0043 fix ipa-dns-install to not require DM password
On Fri, 2011-01-07 at 09:53 +0100, Jan Zelený wrote: > Simo Sorce wrote: > > On Thu, 2011-01-06 at 10:35 +0100, Jan Zelený wrote: > > > Simo Sorce wrote: > > > > This patch makes it possible to run ipa-dns-install and use the admin > > > > kerberos credentials. > > > > > > > > Fixes #686. > > > > > > > > Simo. > > > > > > Nack, I have some comments: > > > > > > Exception handling (chunk #4): > > > Those prints should go away. But the main thing: that particular part of > > > code doesn't seem to produce any exceptions, which should be handled > > > > > > Function ldap_disconnect isn't used anywhere. That makes me wonder - is > > > it redundant or should it be somewhere in the code. I guess this is a > > > policy issue - either we want the connection to stay as long as possible > > > or we want to use it only for a certain set of commands and then > > > disconnect it. > > > > Attached new patch that fixes hunk #4. > > Actually I ended up using ldap_disconnect() here as we need to test the > > ldap connection anyway. > > I also had to do minor changes to Bindinstance() as the code was > > clearing self.fqdn after Service.__init__ set it. > > > > Simo. > > Nack, > IMO in case invalid credentials are given and the script runs in unattended > mode, the script should exit and not ask for the password. Good catch! New patch attached. Simo. binrXefxVEDNB.bin Description: application/mbox ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0043 fix ipa-dns-install to not require DM password
On Thu, 2011-01-06 at 10:35 +0100, Jan Zelený wrote: > Simo Sorce wrote: > > This patch makes it possible to run ipa-dns-install and use the admin > > kerberos credentials. > > > > Fixes #686. > > > > Simo. > > Nack, I have some comments: > > Exception handling (chunk #4): > Those prints should go away. But the main thing: that particular part of code > doesn't seem to produce any exceptions, which should be handled > > Function ldap_disconnect isn't used anywhere. That makes me wonder - is it > redundant or should it be somewhere in the code. I guess this is a policy > issue - either we want the connection to stay as long as possible or we want > to use it only for a certain set of commands and then disconnect it. Attached new patch that fixes hunk #4. Actually I ended up using ldap_disconnect() here as we need to test the ldap connection anyway. I also had to do minor changes to Bindinstance() as the code was clearing self.fqdn after Service.__init__ set it. Simo. binMFCLJMSzS8.bin Description: application/mbox ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0043 fix ipa-dns-install to not require DM password
On Thu, 2011-01-06 at 10:35 +0100, Jan Zelený wrote: > Simo Sorce wrote: > > This patch makes it possible to run ipa-dns-install and use the admin > > kerberos credentials. > > > > Fixes #686. > > > > Simo. > > Nack, I have some comments: > > Exception handling (chunk #4): > Those prints should go away. But the main thing: that particular part of code > doesn't seem to produce any exceptions, which should be handled Ok I will remove that part, it was half debugging code and half to handle code that has been later changed. > Function ldap_disconnect isn't used anywhere. That makes me wonder - is it > redundant or should it be somewhere in the code. I guess this is a policy > issue - either we want the connection to stay as long as possible or we want > to use it only for a certain set of commands and then disconnect it. I initially used it to do connect,op,disconnect, but later decided it was better to let connection live as long as the instance was around. In a future patch we may even move admin_conn to be a global handler so that multiple instances will use just one connection instead of having one pending per-instance type, but I didn't want to go that far. However I didn't remove ldap_disconnect because it will be useful if later on someone needs to change the code to have a temporary connection. I think I may want to use it in the next patch I am working on. I can remove it though and re-add it later if needed, I am ok either way. Simo. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0043 fix ipa-dns-install to not require DM password
On Thu, 2011-01-06 at 09:13 +0100, Jan Zelený wrote: > Simo Sorce wrote: > > This patch makes it possible to run ipa-dns-install and use the admin > > kerberos credentials. > > > > Fixes #686. > > > > Simo. > > The patch doesn't apply on current master - does it depend on some other > patch > or just a small glitch? Almost certainly depends on 0042 as I am touching the same files. I will rebase on top of master locally though, so if it still doesn't apply after 0042 let me know and I'll send updated patches. Simo. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0043 fix ipa-dns-install to not require DM password
This patch makes it possible to run ipa-dns-install and use the admin kerberos credentials. Fixes #686. Simo. bineDa4vQ2Cmr.bin Description: application/mbox ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
