On 10/17/2013 02:21 PM, Jan Cholasta wrote:
Hi,
this patchset contains refactoring of the certificate renewal code,
which will be the base for CA certificate renewal.
The biggest change is a new certmonger CA helper
dogtag-ipa-ca-renew-agent, which replaces
dogtag-ipa-retrieve-agent-submit as well as parts of certmonger
post-commands used in certificate renewal. It provides more flexibility
when doing renewals and allows unified certmonger configuration on both
CA master and clones.
How to test: Test both CA-ful and CA-less server and replica installs
and upgrades, check that certmonger is configured properly and
certificate renewal works (see
https://fedorahosted.org/freeipa/ticket/2803#comment:17 for details).
Dependencies:
freeipa-jcholast-161.3-Fix-certificate-renewal-scripts-to-work-with-separat.patch.
Thanks!
Here are my comments from reading the patches; I haven't tested yet.
161. Looks good
172. Looks good
173.
The second hunk removes `pem_cert` that is later used for
upload_ca_dercert()
174. Looks good
175.
This line in upload_ca_cert() seems redundant:
name = certdb.cacert_name
176. Looks good
177. Looks good
178. Looks good
179.
Note: look if the other calls don't rely on this (replica-install,
ca-install)
180. Looks good
181.
What are those constants you define in the beginning? Why are most not
used? I think you should add a link to some reference.
Nitpick - PEP8 explicitly says to avoid aligning with extra spaces:
http://www.python.org/dev/peps/pep-0008/#pet-peeves
Please use a docstring for documenting what request_cert() does, and
describe the return value.
I don't see the purpose of the `if rc == WAIT_WITH_DELAY` block since
`delay, cookie` gets joined again by the caller.
Wouldn't it be cleaner to always return (rc, output) instead of doing
that [1:] dance?
Did you consider using `traceback.format_exc` for logging errors? Full
tracebacks tend to be more useful than just type & message.
182.
I don't understand why you reordered get_subjectaltname & get_subject,
that makes it hard to see what changed.
183. Looks good
184. Looks OK
185.
ldap_connect: conn.disconnect() should be in a finally clause
retrieve_cert:
- Please use docstrings for documenting functions
- The try: block is too long, NotFound only needs to be handled for
conn.get_entry()
- The `except Exception` block should be removed, or it should just log
and re-raise; the error handling is done in main().
main:
Shouldn't we rather raise an error if CERTMONGER_CA_PROFILE has some
unknown value?
186.
I'm getting lost in all the arguments to dogtag_start_tracking(). Could
you name them when calling it?
187.
I think one changelog entry for all these patches is enough.
188.
Please use docstrings for documenting functions.
I think even with OPERATION_NOT_SUPPORTED_BY_HELPER this should display
some output, like "unknown operation %s".
When store_cert() runs out of attempts, it should not return ISSUED. The
exception should be re-raised so it's logged.
189.
Again, naming arguments in dogtag_start_tracking calls would make them
clearer
190.
Instead of dogtag.install_constants, this should use configured_constants().
191.
Again, please use docstrings for documenting functions.
Or just do the `if` right in main(), I don't think a separate function
is worth it here.
192.
Again, naming dogtag_start_tracking args would make the code clearer.
193.
Same here
194.
I don't see why you reorder the methods, it makes the changes harder to
spot.
195. Looks OK
196.
Again, please use docstrings for documenting functions.
--
Petr³
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
