Re: [Freeipa-devel] [PATCH 0001] Add new parameter --ssh-update to ipa-client-install
Hi, On St, 2016-03-02 at 17:51 +0100, Martin Basti wrote: > > > On 27.02.2016 21:19, Martin Štefany wrote: > > Hi, > > > > I did as Jan suggested, everything is now a new command 'ipa- > > sshupdate', > > (so it's based on Jan's 'ipa-certupdate', yeah, a bit of copy- > > paste), > > rest is based on ipa-client-install's code. I'm not sure if this is > > correct, but you might want to change ipa-client-install to just > > 'import > > ipaclient.ipa_sshupdate' for ssh update, or not - I'm not sure how > > this > > is compatible with 'code deduplication', 're-usage', etc. > > > > Another open point from my side is PEP8 compliance, I've ran the new > > code through pep8 utility with defaults and it's 'OK'. But so is > > code in > > my employer's project and they look slightly 'different', mainly for > > brackets, strings, etc. Please, have a look to that, too, I'm happy > > for > > any guidance. > > > > Martin > > > > On Št, 2016-02-25 at 14:36 +0100, Jan Cholasta wrote: > > > Hi, > > > > > > On 25.2.2016 14:23, Martin Basti wrote: > > > > > > > > > > > > On 22.02.2016 22:13, Martin Štefany wrote: > > > > > Hi, > > > > > > > > > > please, review the attached patch which adds --ssh-update to > > > > > ipa- > > > > > client- > > > > > install. > > > > > > > > > > Ticket:https://fedorahosted.org/freeipa/ticket/2655 > > > > Hello, > > > > thank you for your patch. > > > > Please attach a patch as a file next time. > > > > > > > > I have doubts that this should be part of ipa-client-install, > > > > this > > > > needs > > > > a broader discussion. > > > +1, I think it should be a separate command (ignore my earlier > > > suggestion from Trac to incorporate this into ipa-client-install, > > > I > > > was > > > young and stupid). > > > > > > See client/ipa-certupdate and ipaclient/ipa_certupdate.py for an > > > example > > > of how such a command should be implemented. > > > > > > > > > > > Code comments inline: > > > > > > > > > > --- > > > > > Martin > > > > > > > > > > > From 4974a57f48a0cd48b83724297ae2af572bc530eb Mon Sep 17 > > > > > > 00:00:00 2001 > > > > > From: Martin Stefany > > > > > Date: Mon, 22 Feb 2016 20:58:13 + > > > > > Subject: [PATCH] Add new parameter --ssh-update to ipa-client- > > > > > install > > > > > > > > > > Add a new parameter '--ssh-update' which can be used later > > > > > after > > > > > freeipa > > > > > client is installed to update SSH hostkeys and SSHFP DNS > > > > > records > > > > > for > > > > > host. > > > > > > > > > > https://fedorahosted.org/freeipa/ticket/2655 > > > > > --- > > > > > ipa-client/ipa-install/ipa-client-install | 102 > > > > > +- > > > > > 1 file changed, 99 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa- > > > > > client/ipa- > > > > > install/ipa-client-install > > > > > index > > > > > 789ff591591673744ee3b922e5c0181233ad553c..97adfb6b449fb441bdda > > > > > da89 > > > > > a3b151 > > > > > 33e398ca50 100755 > > > > > --- a/ipa-client/ipa-install/ipa-client-install > > > > > +++ b/ipa-client/ipa-install/ipa-client-install > > > > > @@ -71,6 +71,7 @@ CLIENT_INSTALL_ERROR = 1 > > > > > CLIENT_NOT_CONFIGURED = 2 > > > > > CLIENT_ALREADY_CONFIGURED = 3 > > > > > CLIENT_UNINSTALL_ERROR = 4 # error after restoring > > > > > files/state > > > > > +CLIENT_SSHUPDATE_ERROR = 5 # error during update of SSH > > > > > public > > > > > keys > > > > > > > > > > def parse_options(): > > > > > def validate_ca_cert_file_option(option, opt, value, > > > > > parser): > > > > > @@ -215,6 +216,12 @@ def parse_options(): > > > > >
Re: [Freeipa-devel] [PATCH 0001] Add new parameter --ssh-update to ipa-client-install
Hi,
I did as Jan suggested, everything is now a new command 'ipa-sshupdate',
(so it's based on Jan's 'ipa-certupdate', yeah, a bit of copy-paste),
rest is based on ipa-client-install's code. I'm not sure if this is
correct, but you might want to change ipa-client-install to just 'import
ipaclient.ipa_sshupdate' for ssh update, or not - I'm not sure how this
is compatible with 'code deduplication', 're-usage', etc.
Another open point from my side is PEP8 compliance, I've ran the new
code through pep8 utility with defaults and it's 'OK'. But so is code in
my employer's project and they look slightly 'different', mainly for
brackets, strings, etc. Please, have a look to that, too, I'm happy for
any guidance.
Martin
On Št, 2016-02-25 at 14:36 +0100, Jan Cholasta wrote:
> Hi,
>
> On 25.2.2016 14:23, Martin Basti wrote:
> >
> >
> >
> > On 22.02.2016 22:13, Martin Štefany wrote:
> > >
> > > Hi,
> > >
> > > please, review the attached patch which adds --ssh-update to ipa-
> > > client-
> > > install.
> > >
> > > Ticket:https://fedorahosted.org/freeipa/ticket/2655
> > Hello,
> > thank you for your patch.
> > Please attach a patch as a file next time.
> >
> > I have doubts that this should be part of ipa-client-install, this
> > needs
> > a broader discussion.
> +1, I think it should be a separate command (ignore my earlier
> suggestion from Trac to incorporate this into ipa-client-install, I
> was
> young and stupid).
>
> See client/ipa-certupdate and ipaclient/ipa_certupdate.py for an
> example
> of how such a command should be implemented.
>
> >
> >
> > Code comments inline:
> > >
> > >
> > > ---
> > > Martin
> > >
> > > >
> > > > From 4974a57f48a0cd48b83724297ae2af572bc530eb Mon Sep 17
> > > > 00:00:00 2001
> > > From: Martin Stefany
> > > Date: Mon, 22 Feb 2016 20:58:13 +
> > > Subject: [PATCH] Add new parameter --ssh-update to ipa-client-
> > > install
> > >
> > > Add a new parameter '--ssh-update' which can be used later after
> > > freeipa
> > > client is installed to update SSH hostkeys and SSHFP DNS records
> > > for
> > > host.
> > >
> > > https://fedorahosted.org/freeipa/ticket/2655
> > > ---
> > > ipa-client/ipa-install/ipa-client-install | 102
> > > +-
> > > 1 file changed, 99 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-
> > > client/ipa-
> > > install/ipa-client-install
> > > index
> > > 789ff591591673744ee3b922e5c0181233ad553c..97adfb6b449fb441bddada89
> > > a3b151
> > > 33e398ca50 100755
> > > --- a/ipa-client/ipa-install/ipa-client-install
> > > +++ b/ipa-client/ipa-install/ipa-client-install
> > > @@ -71,6 +71,7 @@ CLIENT_INSTALL_ERROR = 1
> > > CLIENT_NOT_CONFIGURED = 2
> > > CLIENT_ALREADY_CONFIGURED = 3
> > > CLIENT_UNINSTALL_ERROR = 4 # error after restoring files/state
> > > +CLIENT_SSHUPDATE_ERROR = 5 # error during update of SSH public
> > > keys
> > >
> > > def parse_options():
> > > def validate_ca_cert_file_option(option, opt, value,
> > > parser):
> > > @@ -215,6 +216,12 @@ def parse_options():
> > > "be run with --
> > > unattended
> > > option")
> > > parser.add_option_group(uninstall_group)
> > >
> > > +sshupdate_group = OptionGroup(parser, "SSH key update
> > > options")
> > > +sshupdate_group.add_option("--ssh-update", dest="ssh_update",
> > > + action="store_true", default=False,
> > > + help="update local host's SSH public keys
> > > in host
> > > entry and DNS.")
> > > +parser.add_option_group(sshupdate_group)
> > > +
> > > options, args = parser.parse_args()
> > > safe_opts = parser.get_safe_opts(options)
> > >
> > > @@ -840,6 +847,92 @@ def uninstall(options, env):
> > >
> > > return rv
> > >
> > > +def sshupdate(options, env):
> > > +if not is_ipa_client_installed():
> > > +root_logger.error("IPA client is not configured on this
> > > system.")
> > > +retu
