Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dogtaginstance: track server certificate with our renew agent #377

Closed
wants to merge 3 commits into from
Closed

dogtaginstance: track server certificate with our renew agent #377

wants to merge 3 commits into from

Conversation

HonzaCholasta
Copy link
Contributor

@HonzaCholasta HonzaCholasta commented Jan 9, 2017

This patchset is intended to make @simo5's life easier when changing the RA agent certificate location in #314.

renew agent: handle non-replicated certificates

In addition to replicated certificates (Dogtag certificates, RA
certificate), handle non-replicated certificates in
dogtag-ipa-ca-renew-agent as well.

dogtaginstance: track server certificate with our renew agent

Track Dogtag's server certificate with dogtag-ipa-ca-renew-agent instead of
dogtag-ipa-renew-agent.

cainstance: do not configure renewal guard

Do not configure renewal guard for dogtag-ipa-renew-agent, as it is not
used in IPA anymore.

https://fedorahosted.org/freeipa/ticket/5959

Jan Cholasta added 3 commits January 9, 2017 08:33
In addition to replicated certificates (Dogtag certificates, RA
certificate), handle non-replicated certificates in
dogtag-ipa-ca-renew-agent as well.

https://fedorahosted.org/freeipa/ticket/5959
Track Dogtag's server certificate with dogtag-ipa-ca-renew-agent instead of
dogtag-ipa-renew-agent.

https://fedorahosted.org/freeipa/ticket/5959
Do not configure renewal guard for dogtag-ipa-renew-agent, as it is not
used in IPA anymore.

https://fedorahosted.org/freeipa/ticket/5959
@stlaz stlaz self-assigned this Jan 12, 2017
@abbra
Copy link
Contributor

abbra commented Jan 12, 2017

Looks very good to me. ACK from my side.

Copy link
Contributor

@stlaz stlaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only have a small note to the patches, I can supply the patch for that, otherwise LGTM, still need to test it.


def is_renewal_master():
ca = cainstance.CAInstance(host_name=api.env.host)
return ca.is_renewal_master()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CAInstance.is_renewal_master() does not use any of the instance variables, it might be worth making it static.

@stlaz
Copy link
Contributor

stlaz commented Jan 12, 2017

Works fine.

@stlaz
Copy link
Contributor

stlaz commented Jan 12, 2017

I made a patch that makes is_renewal_master and set_renewal_master classmethods on @tiran recommendation. Feel free to push it along or leave it, don't let it slow us down if you don't like it.
http://pastebin.com/dCDTAtnS
ACK

@stlaz stlaz added the ack Pull Request approved, can be merged label Jan 12, 2017
@stlaz
Copy link
Contributor

stlaz commented Jan 13, 2017

This PR can be safely pushed, an unknown upstream contributor with the same github nick as me will later create a PR with the proposed classmethod change.

@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Jan 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
4 participants