[Freeipa-devel] [PATCH 0069] ipa-replica-install support caless install with promotion.

[David Kupka]

On 30/11/15 17:24, Jan Cholasta wrote:
> Hi,
>
> On 27.11.2015 07:57, David Kupka wrote:
>> On 26/11/15 15:22, David Kupka wrote:
>>> On 26/11/15 15:13, David Kupka wrote:
>>>> On 26/11/15 15:01, David Kupka wrote:
>>>>> https://fedorahosted.org/freeipa/ticket/5441
>>>>>
>>>>>
>>>> Replaced accidentally inserted tabs.
>>>>
>>>>
>>>>
>>> Fixed indentation I screwed up when replacing tabs :-/
>
> 1) The deprecated --*_pkcs12 and --*_pin aliases should not be supported
> in ipa-replica-install.
>
>
> 2) This check from ipa-replica-prepare should be added to
> Replica.__init__() as well:
>
>          # If any of the PKCS#12 options are selected, all are required.
>          cert_file_req = (options.dirsrv_cert_files,
> options.http_cert_files)
>          cert_file_opt = (options.pkinit_cert_files,)
>          if any(cert_file_req + cert_file_opt) and not all(cert_file_req):
>              self.option_parser.error(
>                  "--dirsrv-cert-file and --http-cert-file are required
> if any "
>                  "PKCS#12 options are used.")
>
>
> 3) This check from ipa-replica-prepare should be added below the
> pkcs12_info initialization block in promote_check():
>
>          if (options.http_cert_files and options.dirsrv_cert_files and
>              http_ca_cert != dirsrv_ca_cert):
>              raise admintool.ScriptError(
>                  "Apache Server SSL certificate and Directory Server SSL "
>                   "certificate are not signed by the same CA certificate")
>
>
> 4) This check should use the same message as ipa-replica-prepare:
> "Cannot issue certificates: a CA is not installed. Use the
> --http-cert-file, --dirsrv-cert-file options to provide custom
> certificates.":
>
> +            if not options.dirsrv_cert_files:
> +                root_logger.error("The remote master does not have a CA "
> +                                  "installed, can't proceed without
> certs")
> +                sys.exit(3)
>
>
> 5) Please use the common "You cannot specify a --option together with
> replica file" error message here:
>
> +            if any(self.ca.dirsrv_pkcs12_file, self.ca.http_pkcs12_file,
> +                self.ca.pkinit_pkcs12_file):
> +                raise RuntimeError("You cannot provide certificates
> together "
> +                                   "with replica file")
>
>
> 6) Please make the ca_is_enabled argument of install_replica_ds() and
> install_http() mandatory and fill as appropriate when called, it will
> make the code more readable.
>
>
> 7)
>
> $ git diff -U0 | pep8 --diff
> ./ipaserver/install/server/replicainstall.py:99:80: E501 line too long
> (82 > 79 characters)
> ./ipaserver/install/server/replicainstall.py:161:80: E501 line too long
> (82 > 79 characters)
> ./ipaserver/install/server/replicainstall.py:1289:13: E265 block comment
> should start with '# '
> ./ipaserver/install/server/replicainstall.py:1291:17: E125 continuation
> line with same indent as next logical line
> ./ipaserver/install/server/replicainstall.py:1291:17: E128 continuation
> line under-indented for visual indent
>
>
> 8) Nitpicks:
>
> s/ca_configured/ca_is_configured/ in install_replica_ds(), for consistency.
>
> Set ca_enabled = False in the else branch rather than before the if
> statement in promote_check().
>
> Put the "#pylint: disable=no-member" in Replica.__init__() in the same
> spot as it is in Server.__init__().
>
>
> Honza
>
Thank you for review. Updated and rebased patch attached.

-- 
David Kupka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0069.3-ipa-replica-install-support-caless-install-with-prom.patch
Type: text/x-patch
Size: 20697 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151201/e5230b8c/attachment.bin>