<html>
  <head>
    <meta content="text/html; charset=windows-1252"
      http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <p><br>
    </p>
    <br>
    <div class="moz-cite-prefix">On 26.07.2016 17:01, Ariel Barria
      wrote:<br>
    </div>
    <blockquote
cite="mid:CANvnMC2H01-EoAbWe98Xsf64xSR9mMgCJTsFXZcut119MU83CQ@mail.gmail.com"
      type="cite">
      <pre wrap="">Hello everyone.

I send patch for review.

Regards,
</pre>
      <br>
      <fieldset class="mimeAttachmentHeader"></fieldset>
      <br>
    </blockquote>
    Hello, thank you for the patch, but I have a few comments:<br>
    <br>
    1)<br>
    can you please use option --backup-server instead of
    --ipa-backup-server to be consistent with --server (as we don't have
    option --ipa-server)<br>
    <br>
    2)<br>
    values passed by --server option are validated if it is IPA server
    or not, this should happen for backup server(s) too.<br>
    <br>
    But looking to current ipa-client-install, it may be challenging to
    achieve this goal. I'm afraid that you might rather wait until we
    refactor the client code (next release hopefully). But in case you
    are brave enough, I can provide advises, but it will be hell.<br>
    <br>
    3)<br>
    There is a question, if the backup server should be used also for
    krb5.conf or other configs where multiple servers can be specified.
    Probably not. But at least this should be mentioned in manpage that
    this option is used only for SSSD (probably there should be check to
    prevent using --backup-server together with --no-sssd option)<br>
    <br>
    4)<br>
    'man ipa-client-install' should be updated with the new option<br>
    <br>
    5)<br>
    ipa_backup_server allows to specify multiple servers, so the new
    option should be multivalued (and then joined to coma separated list
    into SSSD config)<br>
    <br>
    regards,<br>
    Martin<br>
    <br>
  </body>
</html>