Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
On 05/09/14 10:59, Martin Kosek wrote:
On 09/04/2014 03:09 PM, Jan Cholasta wrote:
Dne 4.9.2014 v 13:40 Martin Kosek napsal(a):
On 09/04/2014 01:19 PM, Jan Cholasta wrote:
Dne 4.9.2014 v 12:31 David Kupka napsal(a):
On 09/03/2014 04:45 PM, Jan Cholasta wrote:
Dne 3.9.2014 v 16:25 David Kupka napsal(a):
On 09/03/2014 04:05 PM, Jan Cholasta wrote:
Dne 3.9.2014 v 12:37 David Kupka napsal(a):
On 09/02/2014 01:56 PM, Jan Cholasta wrote:
Dne 29.8.2014 v 14:34 David Kupka napsal(a):
Hope, I've addressed all the issues (except 9 and 11, inline).
Let's go
for another round :-)
On 08/27/2014 11:05 AM, Jan Cholasta wrote:
Hi,
Dne 25.8.2014 v 15:39 David Kupka napsal(a):
On 08/19/2014 05:44 PM, Rob Crittenden wrote:
David Kupka wrote:
On 08/19/2014 09:58 AM, Martin Kosek wrote:
On 08/19/2014 09:05 AM, David Kupka wrote:
FreeIPA will use certmonger D-Bus API as discussed in this
thread
https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
This change should prevent hard-to-reproduce bugs like
https://fedorahosted.org/freeipa/ticket/4280
Thanks for this effort, the updated certmonger module looks
much
better! This
will help us get rid of the non-standard communication with
certmonger.
Just couple initial comments from me by reading the code:
1) Testing needs fixed version of certmonger, right? This needs
to be
spelled
out right with the patch.
Yes, certmonger 0.75.13 and above should be fine according
ticket
https://fedorahosted.org/certmonger/ticket/36. Added to patch
description.
You should update the spec to set the minimum version as well.
Sure, thanks.
2) Description text in patches is cheap, do not be afraid to
use it
and
describe what you did and why. Link to the ticket is missing in
the
description
as well:
Ok, increased verbosity a bit :-)
Subject: [PATCH] Use certmonger D-Bus API instead of messing
with
its
files.
---
3) get_request_id API:
criteria = (
-('cert_storage_location',
dogtag_constants.ALIAS_DIR,
- certmonger.NPATH),
-('cert_nickname', nickname, None),
+('cert_storage_location',
dogtag_constants.ALIAS_DIR),
+('cert_nickname', nickname),
)
request_id = certmonger.get_request_id(criteria)
Do we want to continue using the "criteria" object or should we
rather
switch
to normal function options? I.e. rather using
request_id = certmonger.get_request_id(cert_nickname=nickname,
cert_storage_location=dogtag_constants.ALIAS_DIR)
? It would look more consistent with other calls. I am just
asking,
not insisting.
I've no preference here. It seems to be a very small change. Has
anyone
a reason to do it one way and not the other?
I think I used this criteria thing to avoid having a bazillion
optional
parameters and for future-proofing. I think at this point the
list is
probably pretty stable, so I'd base it on whether you care about
having
a whole ton of optional parameters or not (it has the
advantage of
self-documenting itself).
The list is probably stable but also really excessive. I don't
think it
would help to have more than dozen optional parameters. So I
prefer to
leave as-is and change it in future if it is wanted.
3) Starting function:
+try:
+ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
skip_output=True)
+except Exception, e:
+root_logger.error('Failed to start certmonger: %s'
% e)
+raise e
I see 2 issues related to this code:
a) Do not call SYSTEMCTL directly. To be platform independent,
rather use
services.knownservices.messagebus.start() that is
overridable by
someone else
porting to non-systemd platforms.
Is there anything that can't be done using
ipalib/ipapython/ipaplatform?
It can't make coffee (yet).
b) In this case, do not use "raise e", but just "raise" to keep
the
exception
stack trace intact for better debugging.
Every day there's something new to learn about python or
FreeIPA.
Both a) and b) should be fixed in other occasions and places.
I found only one occurence of a) issue. Is there some hidden or
are
you
talking about the whole FreeIPA project?
4) Feel free to add yourself to Authors section of this module.
You
refactored
it greatly to earn it :-)
Done.
You already import dbus, why also separately import
DBusException?
Removed, thanks for noticing.
rob
1) The patch needs to be rebased.
I didn't notice the patch is targeted for 4.0. Can you please provide
patches for both ipa-4-0 and ipa-4-1/master?
Attached, 0008-5 works on master/ipa-4-1 and 0008-5-ipa40 works on
ipa-4-0.
There is a little bug in ipa-upgradeconfig in the 4.0 version of the
patch. This is wrong:
for request in requests:
nss_dir, nickname, ca_name, pre_command, post_command, profile
= request
criteria = {
'cert-database': nss_dir,
'cert-nickname': nickname,
'ca-name': ca_name,
'template-profile': profile,
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
On 09/04/2014 03:09 PM, Jan Cholasta wrote:
> Dne 4.9.2014 v 13:40 Martin Kosek napsal(a):
>> On 09/04/2014 01:19 PM, Jan Cholasta wrote:
>>> Dne 4.9.2014 v 12:31 David Kupka napsal(a):
On 09/03/2014 04:45 PM, Jan Cholasta wrote:
> Dne 3.9.2014 v 16:25 David Kupka napsal(a):
>> On 09/03/2014 04:05 PM, Jan Cholasta wrote:
>>> Dne 3.9.2014 v 12:37 David Kupka napsal(a):
On 09/02/2014 01:56 PM, Jan Cholasta wrote:
> Dne 29.8.2014 v 14:34 David Kupka napsal(a):
>> Hope, I've addressed all the issues (except 9 and 11, inline).
>> Let's go
>> for another round :-)
>>
>> On 08/27/2014 11:05 AM, Jan Cholasta wrote:
>>> Hi,
>>>
>>> Dne 25.8.2014 v 15:39 David Kupka napsal(a):
On 08/19/2014 05:44 PM, Rob Crittenden wrote:
> David Kupka wrote:
>> On 08/19/2014 09:58 AM, Martin Kosek wrote:
>>> On 08/19/2014 09:05 AM, David Kupka wrote:
FreeIPA will use certmonger D-Bus API as discussed in this
thread
https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
This change should prevent hard-to-reproduce bugs like
https://fedorahosted.org/freeipa/ticket/4280
>>>
>>> Thanks for this effort, the updated certmonger module looks
>>> much
>>> better! This
>>> will help us get rid of the non-standard communication with
>>> certmonger.
>>>
>>> Just couple initial comments from me by reading the code:
>>>
>>> 1) Testing needs fixed version of certmonger, right? This needs
>>> to be
>>> spelled
>>> out right with the patch.
>> Yes, certmonger 0.75.13 and above should be fine according
>> ticket
>> https://fedorahosted.org/certmonger/ticket/36. Added to patch
>> description.
>
> You should update the spec to set the minimum version as well.
Sure, thanks.
>
>>>
>>> 2) Description text in patches is cheap, do not be afraid to
>>> use it
>>> and
>>> describe what you did and why. Link to the ticket is missing in
>>> the
>>> description
>>> as well:
>> Ok, increased verbosity a bit :-)
>>>
Subject: [PATCH] Use certmonger D-Bus API instead of messing
with
its
files.
---
>>>
>>> 3) get_request_id API:
>>>
criteria = (
-('cert_storage_location',
dogtag_constants.ALIAS_DIR,
- certmonger.NPATH),
-('cert_nickname', nickname, None),
+('cert_storage_location',
dogtag_constants.ALIAS_DIR),
+('cert_nickname', nickname),
)
request_id = certmonger.get_request_id(criteria)
>>>
>>> Do we want to continue using the "criteria" object or should we
>>> rather
>>> switch
>>> to normal function options? I.e. rather using
>>>
>>> request_id = certmonger.get_request_id(cert_nickname=nickname,
>>> cert_storage_location=dogtag_constants.ALIAS_DIR)
>>>
>>> ? It would look more consistent with other calls. I am just
>>> asking,
>>> not insisting.
>> I've no preference here. It seems to be a very small change. Has
>> anyone
>> a reason to do it one way and not the other?
>
> I think I used this criteria thing to avoid having a bazillion
> optional
> parameters and for future-proofing. I think at this point the
> list is
> probably pretty stable, so I'd base it on whether you care about
> having
> a whole ton of optional parameters or not (it has the
> advantage of
> self-documenting itself).
>
The list is probably stable but also really excessive. I don't
think it
would help to have more than dozen optional parameters. So I
prefer to
leave as-is and change it in future if it is wanted.
>>>
>>> 3) Starting function:
>>>
+try:
+ipautil.run([pat
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
Dne 4.9.2014 v 13:40 Martin Kosek napsal(a):
On 09/04/2014 01:19 PM, Jan Cholasta wrote:
Dne 4.9.2014 v 12:31 David Kupka napsal(a):
On 09/03/2014 04:45 PM, Jan Cholasta wrote:
Dne 3.9.2014 v 16:25 David Kupka napsal(a):
On 09/03/2014 04:05 PM, Jan Cholasta wrote:
Dne 3.9.2014 v 12:37 David Kupka napsal(a):
On 09/02/2014 01:56 PM, Jan Cholasta wrote:
Dne 29.8.2014 v 14:34 David Kupka napsal(a):
Hope, I've addressed all the issues (except 9 and 11, inline).
Let's go
for another round :-)
On 08/27/2014 11:05 AM, Jan Cholasta wrote:
Hi,
Dne 25.8.2014 v 15:39 David Kupka napsal(a):
On 08/19/2014 05:44 PM, Rob Crittenden wrote:
David Kupka wrote:
On 08/19/2014 09:58 AM, Martin Kosek wrote:
On 08/19/2014 09:05 AM, David Kupka wrote:
FreeIPA will use certmonger D-Bus API as discussed in this
thread
https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
This change should prevent hard-to-reproduce bugs like
https://fedorahosted.org/freeipa/ticket/4280
Thanks for this effort, the updated certmonger module looks
much
better! This
will help us get rid of the non-standard communication with
certmonger.
Just couple initial comments from me by reading the code:
1) Testing needs fixed version of certmonger, right? This needs
to be
spelled
out right with the patch.
Yes, certmonger 0.75.13 and above should be fine according
ticket
https://fedorahosted.org/certmonger/ticket/36. Added to patch
description.
You should update the spec to set the minimum version as well.
Sure, thanks.
2) Description text in patches is cheap, do not be afraid to
use it
and
describe what you did and why. Link to the ticket is missing in
the
description
as well:
Ok, increased verbosity a bit :-)
Subject: [PATCH] Use certmonger D-Bus API instead of messing
with
its
files.
---
3) get_request_id API:
criteria = (
-('cert_storage_location',
dogtag_constants.ALIAS_DIR,
- certmonger.NPATH),
-('cert_nickname', nickname, None),
+('cert_storage_location',
dogtag_constants.ALIAS_DIR),
+('cert_nickname', nickname),
)
request_id = certmonger.get_request_id(criteria)
Do we want to continue using the "criteria" object or should we
rather
switch
to normal function options? I.e. rather using
request_id = certmonger.get_request_id(cert_nickname=nickname,
cert_storage_location=dogtag_constants.ALIAS_DIR)
? It would look more consistent with other calls. I am just
asking,
not insisting.
I've no preference here. It seems to be a very small change. Has
anyone
a reason to do it one way and not the other?
I think I used this criteria thing to avoid having a bazillion
optional
parameters and for future-proofing. I think at this point the
list is
probably pretty stable, so I'd base it on whether you care about
having
a whole ton of optional parameters or not (it has the
advantage of
self-documenting itself).
The list is probably stable but also really excessive. I don't
think it
would help to have more than dozen optional parameters. So I
prefer to
leave as-is and change it in future if it is wanted.
3) Starting function:
+try:
+ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
skip_output=True)
+except Exception, e:
+root_logger.error('Failed to start certmonger: %s'
% e)
+raise e
I see 2 issues related to this code:
a) Do not call SYSTEMCTL directly. To be platform independent,
rather use
services.knownservices.messagebus.start() that is
overridable by
someone else
porting to non-systemd platforms.
Is there anything that can't be done using
ipalib/ipapython/ipaplatform?
It can't make coffee (yet).
b) In this case, do not use "raise e", but just "raise" to keep
the
exception
stack trace intact for better debugging.
Every day there's something new to learn about python or
FreeIPA.
Both a) and b) should be fixed in other occasions and places.
I found only one occurence of a) issue. Is there some hidden or
are
you
talking about the whole FreeIPA project?
4) Feel free to add yourself to Authors section of this module.
You
refactored
it greatly to earn it :-)
Done.
You already import dbus, why also separately import
DBusException?
Removed, thanks for noticing.
rob
1) The patch needs to be rebased.
I didn't notice the patch is targeted for 4.0. Can you please provide
patches for both ipa-4-0 and ipa-4-1/master?
Attached, 0008-5 works on master/ipa-4-1 and 0008-5-ipa40 works on
ipa-4-0.
There is a little bug in ipa-upgradeconfig in the 4.0 version of the
patch. This is wrong:
for request in requests:
nss_dir, nickname, ca_name, pre_command, post_command, profile
= request
criteria = {
'cert-database': nss_dir,
'cert-nickname': nickname,
'ca-name': ca_name,
'template-profile': profile,
}
It should be:
for nss_dir, nickname, ca_name, pre_command,
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
On 09/04/2014 01:19 PM, Jan Cholasta wrote:
> Dne 4.9.2014 v 12:31 David Kupka napsal(a):
>> On 09/03/2014 04:45 PM, Jan Cholasta wrote:
>>> Dne 3.9.2014 v 16:25 David Kupka napsal(a):
On 09/03/2014 04:05 PM, Jan Cholasta wrote:
> Dne 3.9.2014 v 12:37 David Kupka napsal(a):
>> On 09/02/2014 01:56 PM, Jan Cholasta wrote:
>>> Dne 29.8.2014 v 14:34 David Kupka napsal(a):
Hope, I've addressed all the issues (except 9 and 11, inline).
Let's go
for another round :-)
On 08/27/2014 11:05 AM, Jan Cholasta wrote:
> Hi,
>
> Dne 25.8.2014 v 15:39 David Kupka napsal(a):
>> On 08/19/2014 05:44 PM, Rob Crittenden wrote:
>>> David Kupka wrote:
On 08/19/2014 09:58 AM, Martin Kosek wrote:
> On 08/19/2014 09:05 AM, David Kupka wrote:
>> FreeIPA will use certmonger D-Bus API as discussed in this
>> thread
>> https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> This change should prevent hard-to-reproduce bugs like
>> https://fedorahosted.org/freeipa/ticket/4280
>
> Thanks for this effort, the updated certmonger module looks
> much
> better! This
> will help us get rid of the non-standard communication with
> certmonger.
>
> Just couple initial comments from me by reading the code:
>
> 1) Testing needs fixed version of certmonger, right? This needs
> to be
> spelled
> out right with the patch.
Yes, certmonger 0.75.13 and above should be fine according
ticket
https://fedorahosted.org/certmonger/ticket/36. Added to patch
description.
>>>
>>> You should update the spec to set the minimum version as well.
>> Sure, thanks.
>>>
>
> 2) Description text in patches is cheap, do not be afraid to
> use it
> and
> describe what you did and why. Link to the ticket is missing in
> the
> description
> as well:
Ok, increased verbosity a bit :-)
>
>> Subject: [PATCH] Use certmonger D-Bus API instead of messing
>> with
>> its
>> files.
>>
>> ---
>
> 3) get_request_id API:
>
>>criteria = (
>> -('cert_storage_location',
>> dogtag_constants.ALIAS_DIR,
>> - certmonger.NPATH),
>> -('cert_nickname', nickname, None),
>> +('cert_storage_location',
>> dogtag_constants.ALIAS_DIR),
>> +('cert_nickname', nickname),
>>)
>>request_id = certmonger.get_request_id(criteria)
>
> Do we want to continue using the "criteria" object or should we
> rather
> switch
> to normal function options? I.e. rather using
>
> request_id = certmonger.get_request_id(cert_nickname=nickname,
> cert_storage_location=dogtag_constants.ALIAS_DIR)
>
> ? It would look more consistent with other calls. I am just
> asking,
> not insisting.
I've no preference here. It seems to be a very small change. Has
anyone
a reason to do it one way and not the other?
>>>
>>> I think I used this criteria thing to avoid having a bazillion
>>> optional
>>> parameters and for future-proofing. I think at this point the
>>> list is
>>> probably pretty stable, so I'd base it on whether you care about
>>> having
>>> a whole ton of optional parameters or not (it has the
>>> advantage of
>>> self-documenting itself).
>>>
>> The list is probably stable but also really excessive. I don't
>> think it
>> would help to have more than dozen optional parameters. So I
>> prefer to
>> leave as-is and change it in future if it is wanted.
>
> 3) Starting function:
>
>> +try:
>> +ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
>> skip_output=True)
>> +except Exception, e:
>> +root_logger.error('Failed to start certmonger: %s'
>> % e)
>> +raise e
>
> I see 2 issues related to this code:
> a) Do not call SYSTEMCTL directly.
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
Dne 4.9.2014 v 12:31 David Kupka napsal(a):
On 09/03/2014 04:45 PM, Jan Cholasta wrote:
Dne 3.9.2014 v 16:25 David Kupka napsal(a):
On 09/03/2014 04:05 PM, Jan Cholasta wrote:
Dne 3.9.2014 v 12:37 David Kupka napsal(a):
On 09/02/2014 01:56 PM, Jan Cholasta wrote:
Dne 29.8.2014 v 14:34 David Kupka napsal(a):
Hope, I've addressed all the issues (except 9 and 11, inline).
Let's go
for another round :-)
On 08/27/2014 11:05 AM, Jan Cholasta wrote:
Hi,
Dne 25.8.2014 v 15:39 David Kupka napsal(a):
On 08/19/2014 05:44 PM, Rob Crittenden wrote:
David Kupka wrote:
On 08/19/2014 09:58 AM, Martin Kosek wrote:
On 08/19/2014 09:05 AM, David Kupka wrote:
FreeIPA will use certmonger D-Bus API as discussed in this
thread
https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
This change should prevent hard-to-reproduce bugs like
https://fedorahosted.org/freeipa/ticket/4280
Thanks for this effort, the updated certmonger module looks
much
better! This
will help us get rid of the non-standard communication with
certmonger.
Just couple initial comments from me by reading the code:
1) Testing needs fixed version of certmonger, right? This needs
to be
spelled
out right with the patch.
Yes, certmonger 0.75.13 and above should be fine according
ticket
https://fedorahosted.org/certmonger/ticket/36. Added to patch
description.
You should update the spec to set the minimum version as well.
Sure, thanks.
2) Description text in patches is cheap, do not be afraid to
use it
and
describe what you did and why. Link to the ticket is missing in
the
description
as well:
Ok, increased verbosity a bit :-)
Subject: [PATCH] Use certmonger D-Bus API instead of messing
with
its
files.
---
3) get_request_id API:
criteria = (
-('cert_storage_location',
dogtag_constants.ALIAS_DIR,
- certmonger.NPATH),
-('cert_nickname', nickname, None),
+('cert_storage_location',
dogtag_constants.ALIAS_DIR),
+('cert_nickname', nickname),
)
request_id = certmonger.get_request_id(criteria)
Do we want to continue using the "criteria" object or should we
rather
switch
to normal function options? I.e. rather using
request_id = certmonger.get_request_id(cert_nickname=nickname,
cert_storage_location=dogtag_constants.ALIAS_DIR)
? It would look more consistent with other calls. I am just
asking,
not insisting.
I've no preference here. It seems to be a very small change. Has
anyone
a reason to do it one way and not the other?
I think I used this criteria thing to avoid having a bazillion
optional
parameters and for future-proofing. I think at this point the
list is
probably pretty stable, so I'd base it on whether you care about
having
a whole ton of optional parameters or not (it has the
advantage of
self-documenting itself).
The list is probably stable but also really excessive. I don't
think it
would help to have more than dozen optional parameters. So I
prefer to
leave as-is and change it in future if it is wanted.
3) Starting function:
+try:
+ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
skip_output=True)
+except Exception, e:
+root_logger.error('Failed to start certmonger: %s'
% e)
+raise e
I see 2 issues related to this code:
a) Do not call SYSTEMCTL directly. To be platform independent,
rather use
services.knownservices.messagebus.start() that is
overridable by
someone else
porting to non-systemd platforms.
Is there anything that can't be done using
ipalib/ipapython/ipaplatform?
It can't make coffee (yet).
b) In this case, do not use "raise e", but just "raise" to keep
the
exception
stack trace intact for better debugging.
Every day there's something new to learn about python or
FreeIPA.
Both a) and b) should be fixed in other occasions and places.
I found only one occurence of a) issue. Is there some hidden or
are
you
talking about the whole FreeIPA project?
4) Feel free to add yourself to Authors section of this module.
You
refactored
it greatly to earn it :-)
Done.
You already import dbus, why also separately import
DBusException?
Removed, thanks for noticing.
rob
1) The patch needs to be rebased.
I didn't notice the patch is targeted for 4.0. Can you please provide
patches for both ipa-4-0 and ipa-4-1/master?
Attached, 0008-5 works on master/ipa-4-1 and 0008-5-ipa40 works on
ipa-4-0.
There is a little bug in ipa-upgradeconfig in the 4.0 version of the
patch. This is wrong:
for request in requests:
nss_dir, nickname, ca_name, pre_command, post_command, profile
= request
criteria = {
'cert-database': nss_dir,
'cert-nickname': nickname,
'ca-name': ca_name,
'template-profile': profile,
}
It should be:
for nss_dir, nickname, ca_name, pre_command, post_command in
requests:
criteria = {
'cert-database': nss_dir,
'cer
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
On 09/03/2014 04:45 PM, Jan Cholasta wrote:
Dne 3.9.2014 v 16:25 David Kupka napsal(a):
On 09/03/2014 04:05 PM, Jan Cholasta wrote:
Dne 3.9.2014 v 12:37 David Kupka napsal(a):
On 09/02/2014 01:56 PM, Jan Cholasta wrote:
Dne 29.8.2014 v 14:34 David Kupka napsal(a):
Hope, I've addressed all the issues (except 9 and 11, inline).
Let's go
for another round :-)
On 08/27/2014 11:05 AM, Jan Cholasta wrote:
Hi,
Dne 25.8.2014 v 15:39 David Kupka napsal(a):
On 08/19/2014 05:44 PM, Rob Crittenden wrote:
David Kupka wrote:
On 08/19/2014 09:58 AM, Martin Kosek wrote:
On 08/19/2014 09:05 AM, David Kupka wrote:
FreeIPA will use certmonger D-Bus API as discussed in this
thread
https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
This change should prevent hard-to-reproduce bugs like
https://fedorahosted.org/freeipa/ticket/4280
Thanks for this effort, the updated certmonger module looks much
better! This
will help us get rid of the non-standard communication with
certmonger.
Just couple initial comments from me by reading the code:
1) Testing needs fixed version of certmonger, right? This needs
to be
spelled
out right with the patch.
Yes, certmonger 0.75.13 and above should be fine according ticket
https://fedorahosted.org/certmonger/ticket/36. Added to patch
description.
You should update the spec to set the minimum version as well.
Sure, thanks.
2) Description text in patches is cheap, do not be afraid to
use it
and
describe what you did and why. Link to the ticket is missing in
the
description
as well:
Ok, increased verbosity a bit :-)
Subject: [PATCH] Use certmonger D-Bus API instead of messing
with
its
files.
---
3) get_request_id API:
criteria = (
-('cert_storage_location',
dogtag_constants.ALIAS_DIR,
- certmonger.NPATH),
-('cert_nickname', nickname, None),
+('cert_storage_location',
dogtag_constants.ALIAS_DIR),
+('cert_nickname', nickname),
)
request_id = certmonger.get_request_id(criteria)
Do we want to continue using the "criteria" object or should we
rather
switch
to normal function options? I.e. rather using
request_id = certmonger.get_request_id(cert_nickname=nickname,
cert_storage_location=dogtag_constants.ALIAS_DIR)
? It would look more consistent with other calls. I am just
asking,
not insisting.
I've no preference here. It seems to be a very small change. Has
anyone
a reason to do it one way and not the other?
I think I used this criteria thing to avoid having a bazillion
optional
parameters and for future-proofing. I think at this point the
list is
probably pretty stable, so I'd base it on whether you care about
having
a whole ton of optional parameters or not (it has the advantage of
self-documenting itself).
The list is probably stable but also really excessive. I don't
think it
would help to have more than dozen optional parameters. So I
prefer to
leave as-is and change it in future if it is wanted.
3) Starting function:
+try:
+ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
skip_output=True)
+except Exception, e:
+root_logger.error('Failed to start certmonger: %s'
% e)
+raise e
I see 2 issues related to this code:
a) Do not call SYSTEMCTL directly. To be platform independent,
rather use
services.knownservices.messagebus.start() that is overridable by
someone else
porting to non-systemd platforms.
Is there anything that can't be done using
ipalib/ipapython/ipaplatform?
It can't make coffee (yet).
b) In this case, do not use "raise e", but just "raise" to keep
the
exception
stack trace intact for better debugging.
Every day there's something new to learn about python or FreeIPA.
Both a) and b) should be fixed in other occasions and places.
I found only one occurence of a) issue. Is there some hidden or
are
you
talking about the whole FreeIPA project?
4) Feel free to add yourself to Authors section of this module.
You
refactored
it greatly to earn it :-)
Done.
You already import dbus, why also separately import DBusException?
Removed, thanks for noticing.
rob
1) The patch needs to be rebased.
I didn't notice the patch is targeted for 4.0. Can you please provide
patches for both ipa-4-0 and ipa-4-1/master?
Attached, 0008-5 works on master/ipa-4-1 and 0008-5-ipa40 works on
ipa-4-0.
There is a little bug in ipa-upgradeconfig in the 4.0 version of the
patch. This is wrong:
for request in requests:
nss_dir, nickname, ca_name, pre_command, post_command, profile
= request
criteria = {
'cert-database': nss_dir,
'cert-nickname': nickname,
'ca-name': ca_name,
'template-profile': profile,
}
It should be:
for nss_dir, nickname, ca_name, pre_command, post_command in
requests:
criteria = {
'cert-database': nss_dir,
'cert-nickname': nickname,
'ca-name':
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
Dne 3.9.2014 v 16:25 David Kupka napsal(a):
On 09/03/2014 04:05 PM, Jan Cholasta wrote:
Dne 3.9.2014 v 12:37 David Kupka napsal(a):
On 09/02/2014 01:56 PM, Jan Cholasta wrote:
Dne 29.8.2014 v 14:34 David Kupka napsal(a):
Hope, I've addressed all the issues (except 9 and 11, inline).
Let's go
for another round :-)
On 08/27/2014 11:05 AM, Jan Cholasta wrote:
Hi,
Dne 25.8.2014 v 15:39 David Kupka napsal(a):
On 08/19/2014 05:44 PM, Rob Crittenden wrote:
David Kupka wrote:
On 08/19/2014 09:58 AM, Martin Kosek wrote:
On 08/19/2014 09:05 AM, David Kupka wrote:
FreeIPA will use certmonger D-Bus API as discussed in this
thread
https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
This change should prevent hard-to-reproduce bugs like
https://fedorahosted.org/freeipa/ticket/4280
Thanks for this effort, the updated certmonger module looks much
better! This
will help us get rid of the non-standard communication with
certmonger.
Just couple initial comments from me by reading the code:
1) Testing needs fixed version of certmonger, right? This needs
to be
spelled
out right with the patch.
Yes, certmonger 0.75.13 and above should be fine according ticket
https://fedorahosted.org/certmonger/ticket/36. Added to patch
description.
You should update the spec to set the minimum version as well.
Sure, thanks.
2) Description text in patches is cheap, do not be afraid to
use it
and
describe what you did and why. Link to the ticket is missing in
the
description
as well:
Ok, increased verbosity a bit :-)
Subject: [PATCH] Use certmonger D-Bus API instead of messing
with
its
files.
---
3) get_request_id API:
criteria = (
-('cert_storage_location',
dogtag_constants.ALIAS_DIR,
- certmonger.NPATH),
-('cert_nickname', nickname, None),
+('cert_storage_location',
dogtag_constants.ALIAS_DIR),
+('cert_nickname', nickname),
)
request_id = certmonger.get_request_id(criteria)
Do we want to continue using the "criteria" object or should we
rather
switch
to normal function options? I.e. rather using
request_id = certmonger.get_request_id(cert_nickname=nickname,
cert_storage_location=dogtag_constants.ALIAS_DIR)
? It would look more consistent with other calls. I am just
asking,
not insisting.
I've no preference here. It seems to be a very small change. Has
anyone
a reason to do it one way and not the other?
I think I used this criteria thing to avoid having a bazillion
optional
parameters and for future-proofing. I think at this point the
list is
probably pretty stable, so I'd base it on whether you care about
having
a whole ton of optional parameters or not (it has the advantage of
self-documenting itself).
The list is probably stable but also really excessive. I don't
think it
would help to have more than dozen optional parameters. So I
prefer to
leave as-is and change it in future if it is wanted.
3) Starting function:
+try:
+ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
skip_output=True)
+except Exception, e:
+root_logger.error('Failed to start certmonger: %s' % e)
+raise e
I see 2 issues related to this code:
a) Do not call SYSTEMCTL directly. To be platform independent,
rather use
services.knownservices.messagebus.start() that is overridable by
someone else
porting to non-systemd platforms.
Is there anything that can't be done using
ipalib/ipapython/ipaplatform?
It can't make coffee (yet).
b) In this case, do not use "raise e", but just "raise" to keep
the
exception
stack trace intact for better debugging.
Every day there's something new to learn about python or FreeIPA.
Both a) and b) should be fixed in other occasions and places.
I found only one occurence of a) issue. Is there some hidden or
are
you
talking about the whole FreeIPA project?
4) Feel free to add yourself to Authors section of this module.
You
refactored
it greatly to earn it :-)
Done.
You already import dbus, why also separately import DBusException?
Removed, thanks for noticing.
rob
1) The patch needs to be rebased.
I didn't notice the patch is targeted for 4.0. Can you please provide
patches for both ipa-4-0 and ipa-4-1/master?
Attached, 0008-5 works on master/ipa-4-1 and 0008-5-ipa40 works on
ipa-4-0.
There is a little bug in ipa-upgradeconfig in the 4.0 version of the
patch. This is wrong:
for request in requests:
nss_dir, nickname, ca_name, pre_command, post_command, profile
= request
criteria = {
'cert-database': nss_dir,
'cert-nickname': nickname,
'ca-name': ca_name,
'template-profile': profile,
}
It should be:
for nss_dir, nickname, ca_name, pre_command, post_command in
requests:
criteria = {
'cert-database': nss_dir,
'cert-nickname': nickname,
'ca-name': ca_name,
}
2) Please try to fo
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
On 09/03/2014 04:05 PM, Jan Cholasta wrote:
Dne 3.9.2014 v 12:37 David Kupka napsal(a):
On 09/02/2014 01:56 PM, Jan Cholasta wrote:
Dne 29.8.2014 v 14:34 David Kupka napsal(a):
Hope, I've addressed all the issues (except 9 and 11, inline). Let's go
for another round :-)
On 08/27/2014 11:05 AM, Jan Cholasta wrote:
Hi,
Dne 25.8.2014 v 15:39 David Kupka napsal(a):
On 08/19/2014 05:44 PM, Rob Crittenden wrote:
David Kupka wrote:
On 08/19/2014 09:58 AM, Martin Kosek wrote:
On 08/19/2014 09:05 AM, David Kupka wrote:
FreeIPA will use certmonger D-Bus API as discussed in this thread
https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
This change should prevent hard-to-reproduce bugs like
https://fedorahosted.org/freeipa/ticket/4280
Thanks for this effort, the updated certmonger module looks much
better! This
will help us get rid of the non-standard communication with
certmonger.
Just couple initial comments from me by reading the code:
1) Testing needs fixed version of certmonger, right? This needs
to be
spelled
out right with the patch.
Yes, certmonger 0.75.13 and above should be fine according ticket
https://fedorahosted.org/certmonger/ticket/36. Added to patch
description.
You should update the spec to set the minimum version as well.
Sure, thanks.
2) Description text in patches is cheap, do not be afraid to
use it
and
describe what you did and why. Link to the ticket is missing in
the
description
as well:
Ok, increased verbosity a bit :-)
Subject: [PATCH] Use certmonger D-Bus API instead of messing with
its
files.
---
3) get_request_id API:
criteria = (
-('cert_storage_location',
dogtag_constants.ALIAS_DIR,
- certmonger.NPATH),
-('cert_nickname', nickname, None),
+('cert_storage_location',
dogtag_constants.ALIAS_DIR),
+('cert_nickname', nickname),
)
request_id = certmonger.get_request_id(criteria)
Do we want to continue using the "criteria" object or should we
rather
switch
to normal function options? I.e. rather using
request_id = certmonger.get_request_id(cert_nickname=nickname,
cert_storage_location=dogtag_constants.ALIAS_DIR)
? It would look more consistent with other calls. I am just
asking,
not insisting.
I've no preference here. It seems to be a very small change. Has
anyone
a reason to do it one way and not the other?
I think I used this criteria thing to avoid having a bazillion
optional
parameters and for future-proofing. I think at this point the
list is
probably pretty stable, so I'd base it on whether you care about
having
a whole ton of optional parameters or not (it has the advantage of
self-documenting itself).
The list is probably stable but also really excessive. I don't
think it
would help to have more than dozen optional parameters. So I
prefer to
leave as-is and change it in future if it is wanted.
3) Starting function:
+try:
+ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
skip_output=True)
+except Exception, e:
+root_logger.error('Failed to start certmonger: %s' % e)
+raise e
I see 2 issues related to this code:
a) Do not call SYSTEMCTL directly. To be platform independent,
rather use
services.knownservices.messagebus.start() that is overridable by
someone else
porting to non-systemd platforms.
Is there anything that can't be done using
ipalib/ipapython/ipaplatform?
It can't make coffee (yet).
b) In this case, do not use "raise e", but just "raise" to keep
the
exception
stack trace intact for better debugging.
Every day there's something new to learn about python or FreeIPA.
Both a) and b) should be fixed in other occasions and places.
I found only one occurence of a) issue. Is there some hidden or are
you
talking about the whole FreeIPA project?
4) Feel free to add yourself to Authors section of this module.
You
refactored
it greatly to earn it :-)
Done.
You already import dbus, why also separately import DBusException?
Removed, thanks for noticing.
rob
1) The patch needs to be rebased.
I didn't notice the patch is targeted for 4.0. Can you please provide
patches for both ipa-4-0 and ipa-4-1/master?
Attached, 0008-5 works on master/ipa-4-1 and 0008-5-ipa40 works on
ipa-4-0.
There is a little bug in ipa-upgradeconfig in the 4.0 version of the
patch. This is wrong:
for request in requests:
nss_dir, nickname, ca_name, pre_command, post_command, profile
= request
criteria = {
'cert-database': nss_dir,
'cert-nickname': nickname,
'ca-name': ca_name,
'template-profile': profile,
}
It should be:
for nss_dir, nickname, ca_name, pre_command, post_command in
requests:
criteria = {
'cert-database': nss_dir,
'cert-nickname': nickname,
'ca-name': ca_name,
}
2) Please try to follow PEP8, at least in certmonger.py.
3) In
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
Dne 3.9.2014 v 12:37 David Kupka napsal(a):
On 09/02/2014 01:56 PM, Jan Cholasta wrote:
Dne 29.8.2014 v 14:34 David Kupka napsal(a):
Hope, I've addressed all the issues (except 9 and 11, inline). Let's go
for another round :-)
On 08/27/2014 11:05 AM, Jan Cholasta wrote:
Hi,
Dne 25.8.2014 v 15:39 David Kupka napsal(a):
On 08/19/2014 05:44 PM, Rob Crittenden wrote:
David Kupka wrote:
On 08/19/2014 09:58 AM, Martin Kosek wrote:
On 08/19/2014 09:05 AM, David Kupka wrote:
FreeIPA will use certmonger D-Bus API as discussed in this thread
https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
This change should prevent hard-to-reproduce bugs like
https://fedorahosted.org/freeipa/ticket/4280
Thanks for this effort, the updated certmonger module looks much
better! This
will help us get rid of the non-standard communication with
certmonger.
Just couple initial comments from me by reading the code:
1) Testing needs fixed version of certmonger, right? This needs
to be
spelled
out right with the patch.
Yes, certmonger 0.75.13 and above should be fine according ticket
https://fedorahosted.org/certmonger/ticket/36. Added to patch
description.
You should update the spec to set the minimum version as well.
Sure, thanks.
2) Description text in patches is cheap, do not be afraid to use it
and
describe what you did and why. Link to the ticket is missing in the
description
as well:
Ok, increased verbosity a bit :-)
Subject: [PATCH] Use certmonger D-Bus API instead of messing with
its
files.
---
3) get_request_id API:
criteria = (
-('cert_storage_location', dogtag_constants.ALIAS_DIR,
- certmonger.NPATH),
-('cert_nickname', nickname, None),
+('cert_storage_location',
dogtag_constants.ALIAS_DIR),
+('cert_nickname', nickname),
)
request_id = certmonger.get_request_id(criteria)
Do we want to continue using the "criteria" object or should we
rather
switch
to normal function options? I.e. rather using
request_id = certmonger.get_request_id(cert_nickname=nickname,
cert_storage_location=dogtag_constants.ALIAS_DIR)
? It would look more consistent with other calls. I am just asking,
not insisting.
I've no preference here. It seems to be a very small change. Has
anyone
a reason to do it one way and not the other?
I think I used this criteria thing to avoid having a bazillion
optional
parameters and for future-proofing. I think at this point the list is
probably pretty stable, so I'd base it on whether you care about
having
a whole ton of optional parameters or not (it has the advantage of
self-documenting itself).
The list is probably stable but also really excessive. I don't
think it
would help to have more than dozen optional parameters. So I prefer to
leave as-is and change it in future if it is wanted.
3) Starting function:
+try:
+ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
skip_output=True)
+except Exception, e:
+root_logger.error('Failed to start certmonger: %s' % e)
+raise e
I see 2 issues related to this code:
a) Do not call SYSTEMCTL directly. To be platform independent,
rather use
services.knownservices.messagebus.start() that is overridable by
someone else
porting to non-systemd platforms.
Is there anything that can't be done using
ipalib/ipapython/ipaplatform?
It can't make coffee (yet).
b) In this case, do not use "raise e", but just "raise" to keep the
exception
stack trace intact for better debugging.
Every day there's something new to learn about python or FreeIPA.
Both a) and b) should be fixed in other occasions and places.
I found only one occurence of a) issue. Is there some hidden or are
you
talking about the whole FreeIPA project?
4) Feel free to add yourself to Authors section of this module. You
refactored
it greatly to earn it :-)
Done.
You already import dbus, why also separately import DBusException?
Removed, thanks for noticing.
rob
1) The patch needs to be rebased.
I didn't notice the patch is targeted for 4.0. Can you please provide
patches for both ipa-4-0 and ipa-4-1/master?
Attached, 0008-5 works on master/ipa-4-1 and 0008-5-ipa40 works on ipa-4-0.
There is a little bug in ipa-upgradeconfig in the 4.0 version of the
patch. This is wrong:
for request in requests:
nss_dir, nickname, ca_name, pre_command, post_command, profile
= request
criteria = {
'cert-database': nss_dir,
'cert-nickname': nickname,
'ca-name': ca_name,
'template-profile': profile,
}
It should be:
for nss_dir, nickname, ca_name, pre_command, post_command in requests:
criteria = {
'cert-database': nss_dir,
'cert-nickname': nickname,
'ca-name': ca_name,
}
2) Please try to follow PEP8, at least in certmonger.py.
3) In certificate_renewal_update() in ipa-upgradeconfig you re
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
On Wed, Sep 03, 2014 at 02:34:44PM +0200, Martin Kosek wrote: > On 09/03/2014 02:07 PM, Jan Cholasta wrote: > > I was about to ask the same. Another option is to ask Nalin to update > > certmonger in F20. > > CCing Nalin. What is your take on this, do you plan to release it to F20. > AFAIK, it is just stabilization/bugfixing release so it should fit there > nicely. Assuming you don't hit any new bugs, yeah, that makes sense to me, too. The current F21 candidate build (not in bodhi yet... I should get to that) is probably what you'll see pop up for F20 as well. HTH, Nalin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
On 09/03/2014 02:07 PM, Jan Cholasta wrote:
> Dne 3.9.2014 v 12:45 Martin Kosek napsal(a):
>> On 09/03/2014 12:37 PM, David Kupka wrote:
>>> On 09/02/2014 01:56 PM, Jan Cholasta wrote:
Dne 29.8.2014 v 14:34 David Kupka napsal(a):
> Hope, I've addressed all the issues (except 9 and 11, inline). Let's go
> for another round :-)
>
> On 08/27/2014 11:05 AM, Jan Cholasta wrote:
>> Hi,
>>
>> Dne 25.8.2014 v 15:39 David Kupka napsal(a):
>>> On 08/19/2014 05:44 PM, Rob Crittenden wrote:
David Kupka wrote:
> On 08/19/2014 09:58 AM, Martin Kosek wrote:
>> On 08/19/2014 09:05 AM, David Kupka wrote:
>>> FreeIPA will use certmonger D-Bus API as discussed in this thread
>>> https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
>>>
>>>
>>>
>>> This change should prevent hard-to-reproduce bugs like
>>> https://fedorahosted.org/freeipa/ticket/4280
>>
>> Thanks for this effort, the updated certmonger module looks much
>> better! This
>> will help us get rid of the non-standard communication with
>> certmonger.
>>
>> Just couple initial comments from me by reading the code:
>>
>> 1) Testing needs fixed version of certmonger, right? This needs
>> to be
>> spelled
>> out right with the patch.
> Yes, certmonger 0.75.13 and above should be fine according ticket
> https://fedorahosted.org/certmonger/ticket/36. Added to patch
> description.
You should update the spec to set the minimum version as well.
>>> Sure, thanks.
>>
>> 2) Description text in patches is cheap, do not be afraid to use it
>> and
>> describe what you did and why. Link to the ticket is missing in the
>> description
>> as well:
> Ok, increased verbosity a bit :-)
>>
>>> Subject: [PATCH] Use certmonger D-Bus API instead of messing with
>>> its
>>> files.
>>>
>>> ---
>>
>> 3) get_request_id API:
>>
>>> criteria = (
>>> -('cert_storage_location', dogtag_constants.ALIAS_DIR,
>>> - certmonger.NPATH),
>>> -('cert_nickname', nickname, None),
>>> +('cert_storage_location', dogtag_constants.ALIAS_DIR),
>>> +('cert_nickname', nickname),
>>> )
>>> request_id = certmonger.get_request_id(criteria)
>>
>> Do we want to continue using the "criteria" object or should we
>> rather
>> switch
>> to normal function options? I.e. rather using
>>
>> request_id = certmonger.get_request_id(cert_nickname=nickname,
>> cert_storage_location=dogtag_constants.ALIAS_DIR)
>>
>> ? It would look more consistent with other calls. I am just asking,
>> not insisting.
> I've no preference here. It seems to be a very small change. Has
> anyone
> a reason to do it one way and not the other?
I think I used this criteria thing to avoid having a bazillion
optional
parameters and for future-proofing. I think at this point the list is
probably pretty stable, so I'd base it on whether you care about
having
a whole ton of optional parameters or not (it has the advantage of
self-documenting itself).
>>> The list is probably stable but also really excessive. I don't think it
>>> would help to have more than dozen optional parameters. So I prefer to
>>> leave as-is and change it in future if it is wanted.
>>
>> 3) Starting function:
>>
>>> +try:
>>> +ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
>>> skip_output=True)
>>> +except Exception, e:
>>> +root_logger.error('Failed to start certmonger: %s' % e)
>>> +raise e
>>
>> I see 2 issues related to this code:
>> a) Do not call SYSTEMCTL directly. To be platform independent,
>> rather use
>> services.knownservices.messagebus.start() that is overridable by
>> someone else
>> porting to non-systemd platforms.
> Is there anything that can't be done using
> ipalib/ipapython/ipaplatform?
It can't make coffee (yet).
>> b) In this case, do not use "raise e", but just "raise" to keep the
>> exception
>> stack trace intact for better debugging.
> Every day there's something new to learn about python or FreeIPA.
>>
>> Both a) and b) should be fixed in other occasions and places.
> I found only one occurence of a) issue. Is there some hidden or are
>>
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
Dne 3.9.2014 v 12:45 Martin Kosek napsal(a):
On 09/03/2014 12:37 PM, David Kupka wrote:
On 09/02/2014 01:56 PM, Jan Cholasta wrote:
Dne 29.8.2014 v 14:34 David Kupka napsal(a):
Hope, I've addressed all the issues (except 9 and 11, inline). Let's go
for another round :-)
On 08/27/2014 11:05 AM, Jan Cholasta wrote:
Hi,
Dne 25.8.2014 v 15:39 David Kupka napsal(a):
On 08/19/2014 05:44 PM, Rob Crittenden wrote:
David Kupka wrote:
On 08/19/2014 09:58 AM, Martin Kosek wrote:
On 08/19/2014 09:05 AM, David Kupka wrote:
FreeIPA will use certmonger D-Bus API as discussed in this thread
https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
This change should prevent hard-to-reproduce bugs like
https://fedorahosted.org/freeipa/ticket/4280
Thanks for this effort, the updated certmonger module looks much
better! This
will help us get rid of the non-standard communication with
certmonger.
Just couple initial comments from me by reading the code:
1) Testing needs fixed version of certmonger, right? This needs
to be
spelled
out right with the patch.
Yes, certmonger 0.75.13 and above should be fine according ticket
https://fedorahosted.org/certmonger/ticket/36. Added to patch
description.
You should update the spec to set the minimum version as well.
Sure, thanks.
2) Description text in patches is cheap, do not be afraid to use it
and
describe what you did and why. Link to the ticket is missing in the
description
as well:
Ok, increased verbosity a bit :-)
Subject: [PATCH] Use certmonger D-Bus API instead of messing with
its
files.
---
3) get_request_id API:
criteria = (
-('cert_storage_location', dogtag_constants.ALIAS_DIR,
- certmonger.NPATH),
-('cert_nickname', nickname, None),
+('cert_storage_location', dogtag_constants.ALIAS_DIR),
+('cert_nickname', nickname),
)
request_id = certmonger.get_request_id(criteria)
Do we want to continue using the "criteria" object or should we
rather
switch
to normal function options? I.e. rather using
request_id = certmonger.get_request_id(cert_nickname=nickname,
cert_storage_location=dogtag_constants.ALIAS_DIR)
? It would look more consistent with other calls. I am just asking,
not insisting.
I've no preference here. It seems to be a very small change. Has
anyone
a reason to do it one way and not the other?
I think I used this criteria thing to avoid having a bazillion
optional
parameters and for future-proofing. I think at this point the list is
probably pretty stable, so I'd base it on whether you care about
having
a whole ton of optional parameters or not (it has the advantage of
self-documenting itself).
The list is probably stable but also really excessive. I don't think it
would help to have more than dozen optional parameters. So I prefer to
leave as-is and change it in future if it is wanted.
3) Starting function:
+try:
+ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
skip_output=True)
+except Exception, e:
+root_logger.error('Failed to start certmonger: %s' % e)
+raise e
I see 2 issues related to this code:
a) Do not call SYSTEMCTL directly. To be platform independent,
rather use
services.knownservices.messagebus.start() that is overridable by
someone else
porting to non-systemd platforms.
Is there anything that can't be done using
ipalib/ipapython/ipaplatform?
It can't make coffee (yet).
b) In this case, do not use "raise e", but just "raise" to keep the
exception
stack trace intact for better debugging.
Every day there's something new to learn about python or FreeIPA.
Both a) and b) should be fixed in other occasions and places.
I found only one occurence of a) issue. Is there some hidden or are
you
talking about the whole FreeIPA project?
4) Feel free to add yourself to Authors section of this module. You
refactored
it greatly to earn it :-)
Done.
You already import dbus, why also separately import DBusException?
Removed, thanks for noticing.
rob
1) The patch needs to be rebased.
I didn't notice the patch is targeted for 4.0. Can you please provide
patches for both ipa-4-0 and ipa-4-1/master?
Attached, 0008-5 works on master/ipa-4-1 and 0008-5-ipa40 works on ipa-4-0.
2) Please try to follow PEP8, at least in certmonger.py.
3) In certificate_renewal_update() in ipa-upgradeconfig you removed
ca_name from criteria.
4) IMO renaming nickname to cert_nickname in dogtag_start_tracking() and
stop_tracking() is unnecessary. We can keep calling request nicknames
"request_id" and certificate nicknames "nickname" in our APIs.
5) I think it would be better to add a docstring to
_cm_dbus_object.__init__() instead of doing this:
# object is accesible over this DBus bus instance
bus = None
# DBus object path
path = None
# the actual DBus object
obj = None
# object interface name
obj_dbus_if = None
# object
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
On 09/03/2014 12:37 PM, David Kupka wrote:
> On 09/02/2014 01:56 PM, Jan Cholasta wrote:
>> Dne 29.8.2014 v 14:34 David Kupka napsal(a):
>>> Hope, I've addressed all the issues (except 9 and 11, inline). Let's go
>>> for another round :-)
>>>
>>> On 08/27/2014 11:05 AM, Jan Cholasta wrote:
Hi,
Dne 25.8.2014 v 15:39 David Kupka napsal(a):
> On 08/19/2014 05:44 PM, Rob Crittenden wrote:
>> David Kupka wrote:
>>> On 08/19/2014 09:58 AM, Martin Kosek wrote:
On 08/19/2014 09:05 AM, David Kupka wrote:
> FreeIPA will use certmonger D-Bus API as discussed in this thread
> https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
>
>
>
> This change should prevent hard-to-reproduce bugs like
> https://fedorahosted.org/freeipa/ticket/4280
Thanks for this effort, the updated certmonger module looks much
better! This
will help us get rid of the non-standard communication with
certmonger.
Just couple initial comments from me by reading the code:
1) Testing needs fixed version of certmonger, right? This needs
to be
spelled
out right with the patch.
>>> Yes, certmonger 0.75.13 and above should be fine according ticket
>>> https://fedorahosted.org/certmonger/ticket/36. Added to patch
>>> description.
>>
>> You should update the spec to set the minimum version as well.
> Sure, thanks.
>>
2) Description text in patches is cheap, do not be afraid to use it
and
describe what you did and why. Link to the ticket is missing in the
description
as well:
>>> Ok, increased verbosity a bit :-)
> Subject: [PATCH] Use certmonger D-Bus API instead of messing with
> its
> files.
>
> ---
3) get_request_id API:
>criteria = (
> -('cert_storage_location', dogtag_constants.ALIAS_DIR,
> - certmonger.NPATH),
> -('cert_nickname', nickname, None),
> +('cert_storage_location', dogtag_constants.ALIAS_DIR),
> +('cert_nickname', nickname),
>)
>request_id = certmonger.get_request_id(criteria)
Do we want to continue using the "criteria" object or should we
rather
switch
to normal function options? I.e. rather using
request_id = certmonger.get_request_id(cert_nickname=nickname,
cert_storage_location=dogtag_constants.ALIAS_DIR)
? It would look more consistent with other calls. I am just asking,
not insisting.
>>> I've no preference here. It seems to be a very small change. Has
>>> anyone
>>> a reason to do it one way and not the other?
>>
>> I think I used this criteria thing to avoid having a bazillion
>> optional
>> parameters and for future-proofing. I think at this point the list is
>> probably pretty stable, so I'd base it on whether you care about
>> having
>> a whole ton of optional parameters or not (it has the advantage of
>> self-documenting itself).
>>
> The list is probably stable but also really excessive. I don't think it
> would help to have more than dozen optional parameters. So I prefer to
> leave as-is and change it in future if it is wanted.
3) Starting function:
> +try:
> +ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
> skip_output=True)
> +except Exception, e:
> +root_logger.error('Failed to start certmonger: %s' % e)
> +raise e
I see 2 issues related to this code:
a) Do not call SYSTEMCTL directly. To be platform independent,
rather use
services.knownservices.messagebus.start() that is overridable by
someone else
porting to non-systemd platforms.
>>> Is there anything that can't be done using
>>> ipalib/ipapython/ipaplatform?
>>
>> It can't make coffee (yet).
>>
b) In this case, do not use "raise e", but just "raise" to keep the
exception
stack trace intact for better debugging.
>>> Every day there's something new to learn about python or FreeIPA.
Both a) and b) should be fixed in other occasions and places.
>>> I found only one occurence of a) issue. Is there some hidden or are
>>> you
>>> talking about the whole FreeIPA project?
4) Feel free to add yourself to Authors section of this module. You
refactored
it greatly to earn it :-)
>>> Done.
>>
>> You already import dbus, why also separately import DBusException?
>>
> Removed, thanks for not
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
On 09/02/2014 01:56 PM, Jan Cholasta wrote:
Dne 29.8.2014 v 14:34 David Kupka napsal(a):
Hope, I've addressed all the issues (except 9 and 11, inline). Let's go
for another round :-)
On 08/27/2014 11:05 AM, Jan Cholasta wrote:
Hi,
Dne 25.8.2014 v 15:39 David Kupka napsal(a):
On 08/19/2014 05:44 PM, Rob Crittenden wrote:
David Kupka wrote:
On 08/19/2014 09:58 AM, Martin Kosek wrote:
On 08/19/2014 09:05 AM, David Kupka wrote:
FreeIPA will use certmonger D-Bus API as discussed in this thread
https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
This change should prevent hard-to-reproduce bugs like
https://fedorahosted.org/freeipa/ticket/4280
Thanks for this effort, the updated certmonger module looks much
better! This
will help us get rid of the non-standard communication with
certmonger.
Just couple initial comments from me by reading the code:
1) Testing needs fixed version of certmonger, right? This needs
to be
spelled
out right with the patch.
Yes, certmonger 0.75.13 and above should be fine according ticket
https://fedorahosted.org/certmonger/ticket/36. Added to patch
description.
You should update the spec to set the minimum version as well.
Sure, thanks.
2) Description text in patches is cheap, do not be afraid to use it
and
describe what you did and why. Link to the ticket is missing in the
description
as well:
Ok, increased verbosity a bit :-)
Subject: [PATCH] Use certmonger D-Bus API instead of messing with
its
files.
---
3) get_request_id API:
criteria = (
-('cert_storage_location', dogtag_constants.ALIAS_DIR,
- certmonger.NPATH),
-('cert_nickname', nickname, None),
+('cert_storage_location', dogtag_constants.ALIAS_DIR),
+('cert_nickname', nickname),
)
request_id = certmonger.get_request_id(criteria)
Do we want to continue using the "criteria" object or should we
rather
switch
to normal function options? I.e. rather using
request_id = certmonger.get_request_id(cert_nickname=nickname,
cert_storage_location=dogtag_constants.ALIAS_DIR)
? It would look more consistent with other calls. I am just asking,
not insisting.
I've no preference here. It seems to be a very small change. Has
anyone
a reason to do it one way and not the other?
I think I used this criteria thing to avoid having a bazillion
optional
parameters and for future-proofing. I think at this point the list is
probably pretty stable, so I'd base it on whether you care about
having
a whole ton of optional parameters or not (it has the advantage of
self-documenting itself).
The list is probably stable but also really excessive. I don't think it
would help to have more than dozen optional parameters. So I prefer to
leave as-is and change it in future if it is wanted.
3) Starting function:
+try:
+ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
skip_output=True)
+except Exception, e:
+root_logger.error('Failed to start certmonger: %s' % e)
+raise e
I see 2 issues related to this code:
a) Do not call SYSTEMCTL directly. To be platform independent,
rather use
services.knownservices.messagebus.start() that is overridable by
someone else
porting to non-systemd platforms.
Is there anything that can't be done using
ipalib/ipapython/ipaplatform?
It can't make coffee (yet).
b) In this case, do not use "raise e", but just "raise" to keep the
exception
stack trace intact for better debugging.
Every day there's something new to learn about python or FreeIPA.
Both a) and b) should be fixed in other occasions and places.
I found only one occurence of a) issue. Is there some hidden or are
you
talking about the whole FreeIPA project?
4) Feel free to add yourself to Authors section of this module. You
refactored
it greatly to earn it :-)
Done.
You already import dbus, why also separately import DBusException?
Removed, thanks for noticing.
rob
1) The patch needs to be rebased.
I didn't notice the patch is targeted for 4.0. Can you please provide
patches for both ipa-4-0 and ipa-4-1/master?
Attached, 0008-5 works on master/ipa-4-1 and 0008-5-ipa40 works on ipa-4-0.
2) Please try to follow PEP8, at least in certmonger.py.
3) In certificate_renewal_update() in ipa-upgradeconfig you removed
ca_name from criteria.
4) IMO renaming nickname to cert_nickname in dogtag_start_tracking() and
stop_tracking() is unnecessary. We can keep calling request nicknames
"request_id" and certificate nicknames "nickname" in our APIs.
5) I think it would be better to add a docstring to
_cm_dbus_object.__init__() instead of doing this:
# object is accesible over this DBus bus instance
bus = None
# DBus object path
path = None
# the actual DBus object
obj = None
# object interface name
obj_dbus_if = None
# object parent interface name
parent_dbus_if = None
# object inteface
obj_if = None
# prop
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
Dne 29.8.2014 v 14:34 David Kupka napsal(a):
Hope, I've addressed all the issues (except 9 and 11, inline). Let's go
for another round :-)
On 08/27/2014 11:05 AM, Jan Cholasta wrote:
Hi,
Dne 25.8.2014 v 15:39 David Kupka napsal(a):
On 08/19/2014 05:44 PM, Rob Crittenden wrote:
David Kupka wrote:
On 08/19/2014 09:58 AM, Martin Kosek wrote:
On 08/19/2014 09:05 AM, David Kupka wrote:
FreeIPA will use certmonger D-Bus API as discussed in this thread
https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
This change should prevent hard-to-reproduce bugs like
https://fedorahosted.org/freeipa/ticket/4280
Thanks for this effort, the updated certmonger module looks much
better! This
will help us get rid of the non-standard communication with
certmonger.
Just couple initial comments from me by reading the code:
1) Testing needs fixed version of certmonger, right? This needs to be
spelled
out right with the patch.
Yes, certmonger 0.75.13 and above should be fine according ticket
https://fedorahosted.org/certmonger/ticket/36. Added to patch
description.
You should update the spec to set the minimum version as well.
Sure, thanks.
2) Description text in patches is cheap, do not be afraid to use it
and
describe what you did and why. Link to the ticket is missing in the
description
as well:
Ok, increased verbosity a bit :-)
Subject: [PATCH] Use certmonger D-Bus API instead of messing with
its
files.
---
3) get_request_id API:
criteria = (
-('cert_storage_location', dogtag_constants.ALIAS_DIR,
- certmonger.NPATH),
-('cert_nickname', nickname, None),
+('cert_storage_location', dogtag_constants.ALIAS_DIR),
+('cert_nickname', nickname),
)
request_id = certmonger.get_request_id(criteria)
Do we want to continue using the "criteria" object or should we
rather
switch
to normal function options? I.e. rather using
request_id = certmonger.get_request_id(cert_nickname=nickname,
cert_storage_location=dogtag_constants.ALIAS_DIR)
? It would look more consistent with other calls. I am just asking,
not insisting.
I've no preference here. It seems to be a very small change. Has
anyone
a reason to do it one way and not the other?
I think I used this criteria thing to avoid having a bazillion optional
parameters and for future-proofing. I think at this point the list is
probably pretty stable, so I'd base it on whether you care about having
a whole ton of optional parameters or not (it has the advantage of
self-documenting itself).
The list is probably stable but also really excessive. I don't think it
would help to have more than dozen optional parameters. So I prefer to
leave as-is and change it in future if it is wanted.
3) Starting function:
+try:
+ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
skip_output=True)
+except Exception, e:
+root_logger.error('Failed to start certmonger: %s' % e)
+raise e
I see 2 issues related to this code:
a) Do not call SYSTEMCTL directly. To be platform independent,
rather use
services.knownservices.messagebus.start() that is overridable by
someone else
porting to non-systemd platforms.
Is there anything that can't be done using
ipalib/ipapython/ipaplatform?
It can't make coffee (yet).
b) In this case, do not use "raise e", but just "raise" to keep the
exception
stack trace intact for better debugging.
Every day there's something new to learn about python or FreeIPA.
Both a) and b) should be fixed in other occasions and places.
I found only one occurence of a) issue. Is there some hidden or are
you
talking about the whole FreeIPA project?
4) Feel free to add yourself to Authors section of this module. You
refactored
it greatly to earn it :-)
Done.
You already import dbus, why also separately import DBusException?
Removed, thanks for noticing.
rob
1) The patch needs to be rebased.
I didn't notice the patch is targeted for 4.0. Can you please provide
patches for both ipa-4-0 and ipa-4-1/master?
2) Please try to follow PEP8, at least in certmonger.py.
3) In certificate_renewal_update() in ipa-upgradeconfig you removed
ca_name from criteria.
4) IMO renaming nickname to cert_nickname in dogtag_start_tracking() and
stop_tracking() is unnecessary. We can keep calling request nicknames
"request_id" and certificate nicknames "nickname" in our APIs.
5) I think it would be better to add a docstring to
_cm_dbus_object.__init__() instead of doing this:
# object is accesible over this DBus bus instance
bus = None
# DBus object path
path = None
# the actual DBus object
obj = None
# object interface name
obj_dbus_if = None
# object parent interface name
parent_dbus_if = None
# object inteface
obj_if = None
# property interface
prop_if = None
You removed the comments, but left the attributes there. You should
remove them as we
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
Hope, I've addressed all the issues (except 9 and 11, inline). Let's go
for another round :-)
On 08/27/2014 11:05 AM, Jan Cholasta wrote:
Hi,
Dne 25.8.2014 v 15:39 David Kupka napsal(a):
On 08/19/2014 05:44 PM, Rob Crittenden wrote:
David Kupka wrote:
On 08/19/2014 09:58 AM, Martin Kosek wrote:
On 08/19/2014 09:05 AM, David Kupka wrote:
FreeIPA will use certmonger D-Bus API as discussed in this thread
https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
This change should prevent hard-to-reproduce bugs like
https://fedorahosted.org/freeipa/ticket/4280
Thanks for this effort, the updated certmonger module looks much
better! This
will help us get rid of the non-standard communication with
certmonger.
Just couple initial comments from me by reading the code:
1) Testing needs fixed version of certmonger, right? This needs to be
spelled
out right with the patch.
Yes, certmonger 0.75.13 and above should be fine according ticket
https://fedorahosted.org/certmonger/ticket/36. Added to patch
description.
You should update the spec to set the minimum version as well.
Sure, thanks.
2) Description text in patches is cheap, do not be afraid to use it
and
describe what you did and why. Link to the ticket is missing in the
description
as well:
Ok, increased verbosity a bit :-)
Subject: [PATCH] Use certmonger D-Bus API instead of messing with its
files.
---
3) get_request_id API:
criteria = (
-('cert_storage_location', dogtag_constants.ALIAS_DIR,
- certmonger.NPATH),
-('cert_nickname', nickname, None),
+('cert_storage_location', dogtag_constants.ALIAS_DIR),
+('cert_nickname', nickname),
)
request_id = certmonger.get_request_id(criteria)
Do we want to continue using the "criteria" object or should we rather
switch
to normal function options? I.e. rather using
request_id = certmonger.get_request_id(cert_nickname=nickname,
cert_storage_location=dogtag_constants.ALIAS_DIR)
? It would look more consistent with other calls. I am just asking,
not insisting.
I've no preference here. It seems to be a very small change. Has anyone
a reason to do it one way and not the other?
I think I used this criteria thing to avoid having a bazillion optional
parameters and for future-proofing. I think at this point the list is
probably pretty stable, so I'd base it on whether you care about having
a whole ton of optional parameters or not (it has the advantage of
self-documenting itself).
The list is probably stable but also really excessive. I don't think it
would help to have more than dozen optional parameters. So I prefer to
leave as-is and change it in future if it is wanted.
3) Starting function:
+try:
+ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
skip_output=True)
+except Exception, e:
+root_logger.error('Failed to start certmonger: %s' % e)
+raise e
I see 2 issues related to this code:
a) Do not call SYSTEMCTL directly. To be platform independent,
rather use
services.knownservices.messagebus.start() that is overridable by
someone else
porting to non-systemd platforms.
Is there anything that can't be done using
ipalib/ipapython/ipaplatform?
It can't make coffee (yet).
b) In this case, do not use "raise e", but just "raise" to keep the
exception
stack trace intact for better debugging.
Every day there's something new to learn about python or FreeIPA.
Both a) and b) should be fixed in other occasions and places.
I found only one occurence of a) issue. Is there some hidden or are you
talking about the whole FreeIPA project?
4) Feel free to add yourself to Authors section of this module. You
refactored
it greatly to earn it :-)
Done.
You already import dbus, why also separately import DBusException?
Removed, thanks for noticing.
rob
1) The patch needs to be rebased.
2) Please try to follow PEP8, at least in certmonger.py.
3) In certificate_renewal_update() in ipa-upgradeconfig you removed
ca_name from criteria.
4) IMO renaming nickname to cert_nickname in dogtag_start_tracking() and
stop_tracking() is unnecessary. We can keep calling request nicknames
"request_id" and certificate nicknames "nickname" in our APIs.
5) I think it would be better to add a docstring to
_cm_dbus_object.__init__() instead of doing this:
# object is accesible over this DBus bus instance
bus = None
# DBus object path
path = None
# the actual DBus object
obj = None
# object interface name
obj_dbus_if = None
# object parent interface name
parent_dbus_if = None
# object inteface
obj_if = None
# property interface
prop_if = None
6) In _start_certmonger(), please check if certmonger is already running
before starting it, what applies to systemd might not apply to other
init systems.
7) Do we ever need to connect to certmonger on the session bus? If not,
there is no need to sup
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
Hi,
Dne 25.8.2014 v 15:39 David Kupka napsal(a):
On 08/19/2014 05:44 PM, Rob Crittenden wrote:
David Kupka wrote:
On 08/19/2014 09:58 AM, Martin Kosek wrote:
On 08/19/2014 09:05 AM, David Kupka wrote:
FreeIPA will use certmonger D-Bus API as discussed in this thread
https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
This change should prevent hard-to-reproduce bugs like
https://fedorahosted.org/freeipa/ticket/4280
Thanks for this effort, the updated certmonger module looks much
better! This
will help us get rid of the non-standard communication with certmonger.
Just couple initial comments from me by reading the code:
1) Testing needs fixed version of certmonger, right? This needs to be
spelled
out right with the patch.
Yes, certmonger 0.75.13 and above should be fine according ticket
https://fedorahosted.org/certmonger/ticket/36. Added to patch
description.
You should update the spec to set the minimum version as well.
Sure, thanks.
2) Description text in patches is cheap, do not be afraid to use it and
describe what you did and why. Link to the ticket is missing in the
description
as well:
Ok, increased verbosity a bit :-)
Subject: [PATCH] Use certmonger D-Bus API instead of messing with its
files.
---
3) get_request_id API:
criteria = (
-('cert_storage_location', dogtag_constants.ALIAS_DIR,
- certmonger.NPATH),
-('cert_nickname', nickname, None),
+('cert_storage_location', dogtag_constants.ALIAS_DIR),
+('cert_nickname', nickname),
)
request_id = certmonger.get_request_id(criteria)
Do we want to continue using the "criteria" object or should we rather
switch
to normal function options? I.e. rather using
request_id = certmonger.get_request_id(cert_nickname=nickname,
cert_storage_location=dogtag_constants.ALIAS_DIR)
? It would look more consistent with other calls. I am just asking,
not insisting.
I've no preference here. It seems to be a very small change. Has anyone
a reason to do it one way and not the other?
I think I used this criteria thing to avoid having a bazillion optional
parameters and for future-proofing. I think at this point the list is
probably pretty stable, so I'd base it on whether you care about having
a whole ton of optional parameters or not (it has the advantage of
self-documenting itself).
The list is probably stable but also really excessive. I don't think it
would help to have more than dozen optional parameters. So I prefer to
leave as-is and change it in future if it is wanted.
3) Starting function:
+try:
+ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
skip_output=True)
+except Exception, e:
+root_logger.error('Failed to start certmonger: %s' % e)
+raise e
I see 2 issues related to this code:
a) Do not call SYSTEMCTL directly. To be platform independent,
rather use
services.knownservices.messagebus.start() that is overridable by
someone else
porting to non-systemd platforms.
Is there anything that can't be done using ipalib/ipapython/ipaplatform?
It can't make coffee (yet).
b) In this case, do not use "raise e", but just "raise" to keep the
exception
stack trace intact for better debugging.
Every day there's something new to learn about python or FreeIPA.
Both a) and b) should be fixed in other occasions and places.
I found only one occurence of a) issue. Is there some hidden or are you
talking about the whole FreeIPA project?
4) Feel free to add yourself to Authors section of this module. You
refactored
it greatly to earn it :-)
Done.
You already import dbus, why also separately import DBusException?
Removed, thanks for noticing.
rob
1) The patch needs to be rebased.
2) Please try to follow PEP8, at least in certmonger.py.
3) In certificate_renewal_update() in ipa-upgradeconfig you removed
ca_name from criteria.
4) IMO renaming nickname to cert_nickname in dogtag_start_tracking() and
stop_tracking() is unnecessary. We can keep calling request nicknames
"request_id" and certificate nicknames "nickname" in our APIs.
5) I think it would be better to add a docstring to
_cm_dbus_object.__init__() instead of doing this:
# object is accesible over this DBus bus instance
bus = None
# DBus object path
path = None
# the actual DBus object
obj = None
# object interface name
obj_dbus_if = None
# object parent interface name
parent_dbus_if = None
# object inteface
obj_if = None
# property interface
prop_if = None
6) In _start_certmonger(), please check if certmonger is already running
before starting it, what applies to systemd might not apply to other
init systems.
7) Do we ever need to connect to certmonger on the session bus? If not,
there is no need to support it in _connect_to_certmonger().
8) You should not ignore other criteria when 'nickname' is specified in
_get_requests(). Use find_reque
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
On 08/19/2014 05:44 PM, Rob Crittenden wrote:
David Kupka wrote:
On 08/19/2014 09:58 AM, Martin Kosek wrote:
On 08/19/2014 09:05 AM, David Kupka wrote:
FreeIPA will use certmonger D-Bus API as discussed in this thread
https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
This change should prevent hard-to-reproduce bugs like
https://fedorahosted.org/freeipa/ticket/4280
Thanks for this effort, the updated certmonger module looks much
better! This
will help us get rid of the non-standard communication with certmonger.
Just couple initial comments from me by reading the code:
1) Testing needs fixed version of certmonger, right? This needs to be
spelled
out right with the patch.
Yes, certmonger 0.75.13 and above should be fine according ticket
https://fedorahosted.org/certmonger/ticket/36. Added to patch description.
You should update the spec to set the minimum version as well.
Sure, thanks.
2) Description text in patches is cheap, do not be afraid to use it and
describe what you did and why. Link to the ticket is missing in the
description
as well:
Ok, increased verbosity a bit :-)
Subject: [PATCH] Use certmonger D-Bus API instead of messing with its
files.
---
3) get_request_id API:
criteria = (
-('cert_storage_location', dogtag_constants.ALIAS_DIR,
- certmonger.NPATH),
-('cert_nickname', nickname, None),
+('cert_storage_location', dogtag_constants.ALIAS_DIR),
+('cert_nickname', nickname),
)
request_id = certmonger.get_request_id(criteria)
Do we want to continue using the "criteria" object or should we rather
switch
to normal function options? I.e. rather using
request_id = certmonger.get_request_id(cert_nickname=nickname,
cert_storage_location=dogtag_constants.ALIAS_DIR)
? It would look more consistent with other calls. I am just asking,
not insisting.
I've no preference here. It seems to be a very small change. Has anyone
a reason to do it one way and not the other?
I think I used this criteria thing to avoid having a bazillion optional
parameters and for future-proofing. I think at this point the list is
probably pretty stable, so I'd base it on whether you care about having
a whole ton of optional parameters or not (it has the advantage of
self-documenting itself).
The list is probably stable but also really excessive. I don't think it
would help to have more than dozen optional parameters. So I prefer to
leave as-is and change it in future if it is wanted.
3) Starting function:
+try:
+ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
skip_output=True)
+except Exception, e:
+root_logger.error('Failed to start certmonger: %s' % e)
+raise e
I see 2 issues related to this code:
a) Do not call SYSTEMCTL directly. To be platform independent, rather use
services.knownservices.messagebus.start() that is overridable by
someone else
porting to non-systemd platforms.
Is there anything that can't be done using ipalib/ipapython/ipaplatform?
It can't make coffee (yet).
b) In this case, do not use "raise e", but just "raise" to keep the
exception
stack trace intact for better debugging.
Every day there's something new to learn about python or FreeIPA.
Both a) and b) should be fixed in other occasions and places.
I found only one occurence of a) issue. Is there some hidden or are you
talking about the whole FreeIPA project?
4) Feel free to add yourself to Authors section of this module. You
refactored
it greatly to earn it :-)
Done.
You already import dbus, why also separately import DBusException?
Removed, thanks for noticing.
rob
--
David Kupka
From b81786e68fba8efd4bb0c3e86a4702084137e30c Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Wed, 20 Aug 2014 13:58:50 +0200
Subject: [PATCH] Use certmonger D-Bus API instead of messing with its files.
FreeIPA certmonger module changed to use D-Bus to communicate with certmonger.
Using the D-Bus API should be more stable and supported way of using cermonger than
tampering with its files.
> =certmonger-0.75.13 is needed for this to work.
https://fedorahosted.org/freeipa/ticket/4280
---
freeipa.spec.in| 2 +-
install/tools/ipa-upgradeconfig| 13 +-
ipa-client/ipa-install/ipa-client-install | 2 +-
ipa-client/ipaclient/ipa_certupdate.py | 5 +-
ipapython/certmonger.py| 521 -
ipaserver/install/cainstance.py| 10 +-
ipaserver/install/certs.py | 28 +-
ipaserver/install/ipa_cacert_manage.py | 4 +-
ipaserver/install/plugins/ca_renewal_master.py | 4 +-
9 files changed, 273 insertions(+), 316 deletions(-)
diff --git a/freeipa.spec.in b/freeipa.spec.in
index 6f22bc92f76f2e4bd732a995e392d6845dab27b7..15aab5b45c5688f11e0125299c2842f23d986749 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -121,7 +121,7 @
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
On 08/19/2014 05:44 PM, Rob Crittenden wrote: > David Kupka wrote: ... >>> b) In this case, do not use "raise e", but just "raise" to keep the >>> exception >>> stack trace intact for better debugging. >> Every day there's something new to learn about python or FreeIPA. >>> >>> Both a) and b) should be fixed in other occasions and places. >> I found only one occurence of a) issue. Is there some hidden or are you >> talking about the whole FreeIPA project? Ah, I see - there really is just one a). You can just fix the refactored plugin, let us not save the whole world/freeipa yet. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
David Kupka wrote:
> On 08/19/2014 09:58 AM, Martin Kosek wrote:
>> On 08/19/2014 09:05 AM, David Kupka wrote:
>>> FreeIPA will use certmonger D-Bus API as discussed in this thread
>>> https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
>>>
>>> This change should prevent hard-to-reproduce bugs like
>>> https://fedorahosted.org/freeipa/ticket/4280
>>
>> Thanks for this effort, the updated certmonger module looks much
>> better! This
>> will help us get rid of the non-standard communication with certmonger.
>>
>> Just couple initial comments from me by reading the code:
>>
>> 1) Testing needs fixed version of certmonger, right? This needs to be
>> spelled
>> out right with the patch.
> Yes, certmonger 0.75.13 and above should be fine according ticket
> https://fedorahosted.org/certmonger/ticket/36. Added to patch description.
You should update the spec to set the minimum version as well.
>>
>> 2) Description text in patches is cheap, do not be afraid to use it and
>> describe what you did and why. Link to the ticket is missing in the
>> description
>> as well:
> Ok, increased verbosity a bit :-)
>>
>>> Subject: [PATCH] Use certmonger D-Bus API instead of messing with its
>>> files.
>>>
>>> ---
>>
>> 3) get_request_id API:
>>
>>> criteria = (
>>> -('cert_storage_location', dogtag_constants.ALIAS_DIR,
>>> - certmonger.NPATH),
>>> -('cert_nickname', nickname, None),
>>> +('cert_storage_location', dogtag_constants.ALIAS_DIR),
>>> +('cert_nickname', nickname),
>>> )
>>> request_id = certmonger.get_request_id(criteria)
>>
>> Do we want to continue using the "criteria" object or should we rather
>> switch
>> to normal function options? I.e. rather using
>>
>> request_id = certmonger.get_request_id(cert_nickname=nickname,
>> cert_storage_location=dogtag_constants.ALIAS_DIR)
>>
>> ? It would look more consistent with other calls. I am just asking,
>> not insisting.
> I've no preference here. It seems to be a very small change. Has anyone
> a reason to do it one way and not the other?
I think I used this criteria thing to avoid having a bazillion optional
parameters and for future-proofing. I think at this point the list is
probably pretty stable, so I'd base it on whether you care about having
a whole ton of optional parameters or not (it has the advantage of
self-documenting itself).
>>
>> 3) Starting function:
>>
>>> +try:
>>> +ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
>>> skip_output=True)
>>> +except Exception, e:
>>> +root_logger.error('Failed to start certmonger: %s' % e)
>>> +raise e
>>
>> I see 2 issues related to this code:
>> a) Do not call SYSTEMCTL directly. To be platform independent, rather use
>> services.knownservices.messagebus.start() that is overridable by
>> someone else
>> porting to non-systemd platforms.
> Is there anything that can't be done using ipalib/ipapython/ipaplatform?
It can't make coffee (yet).
>> b) In this case, do not use "raise e", but just "raise" to keep the
>> exception
>> stack trace intact for better debugging.
> Every day there's something new to learn about python or FreeIPA.
>>
>> Both a) and b) should be fixed in other occasions and places.
> I found only one occurence of a) issue. Is there some hidden or are you
> talking about the whole FreeIPA project?
>>
>> 4) Feel free to add yourself to Authors section of this module. You
>> refactored
>> it greatly to earn it :-)
> Done.
You already import dbus, why also separately import DBusException?
rob
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
On 08/19/2014 09:58 AM, Martin Kosek wrote:
On 08/19/2014 09:05 AM, David Kupka wrote:
FreeIPA will use certmonger D-Bus API as discussed in this thread
https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
This change should prevent hard-to-reproduce bugs like
https://fedorahosted.org/freeipa/ticket/4280
Thanks for this effort, the updated certmonger module looks much better! This
will help us get rid of the non-standard communication with certmonger.
Just couple initial comments from me by reading the code:
1) Testing needs fixed version of certmonger, right? This needs to be spelled
out right with the patch.
Yes, certmonger 0.75.13 and above should be fine according ticket
https://fedorahosted.org/certmonger/ticket/36. Added to patch description.
2) Description text in patches is cheap, do not be afraid to use it and
describe what you did and why. Link to the ticket is missing in the description
as well:
Ok, increased verbosity a bit :-)
Subject: [PATCH] Use certmonger D-Bus API instead of messing with its files.
---
3) get_request_id API:
criteria = (
-('cert_storage_location', dogtag_constants.ALIAS_DIR,
- certmonger.NPATH),
-('cert_nickname', nickname, None),
+('cert_storage_location', dogtag_constants.ALIAS_DIR),
+('cert_nickname', nickname),
)
request_id = certmonger.get_request_id(criteria)
Do we want to continue using the "criteria" object or should we rather switch
to normal function options? I.e. rather using
request_id = certmonger.get_request_id(cert_nickname=nickname,
cert_storage_location=dogtag_constants.ALIAS_DIR)
? It would look more consistent with other calls. I am just asking, not
insisting.
I've no preference here. It seems to be a very small change. Has anyone
a reason to do it one way and not the other?
3) Starting function:
+try:
+ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'], skip_output=True)
+except Exception, e:
+root_logger.error('Failed to start certmonger: %s' % e)
+raise e
I see 2 issues related to this code:
a) Do not call SYSTEMCTL directly. To be platform independent, rather use
services.knownservices.messagebus.start() that is overridable by someone else
porting to non-systemd platforms.
Is there anything that can't be done using ipalib/ipapython/ipaplatform?
b) In this case, do not use "raise e", but just "raise" to keep the exception
stack trace intact for better debugging.
Every day there's something new to learn about python or FreeIPA.
Both a) and b) should be fixed in other occasions and places.
I found only one occurence of a) issue. Is there some hidden or are you
talking about the whole FreeIPA project?
4) Feel free to add yourself to Authors section of this module. You refactored
it greatly to earn it :-)
Done.
Martin
--
David Kupka
From cd026268d557904bd8fb3ae9642d88eb5c43ac45 Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Tue, 19 Aug 2014 16:34:59 +0200
Subject: [PATCH] Use certmonger D-Bus API instead of messing with its files.
FreeIPA certmonger module changed to use D-Bus to communicate with certmonger.
Using the D-Bus API should be more stable and supported way of using cermonger than
tampering with its files.
>=certmonger-0.75.13 is needed for this to work.
https://fedorahosted.org/freeipa/ticket/4280
---
install/tools/ipa-upgradeconfig| 15 +-
ipa-client/ipa-install/ipa-client-install | 2 +-
ipa-client/ipaclient/ipa_certupdate.py | 5 +-
ipapython/certmonger.py| 522 -
ipaserver/install/cainstance.py| 10 +-
ipaserver/install/certs.py | 28 +-
ipaserver/install/ipa_cacert_manage.py | 4 +-
ipaserver/install/plugins/ca_renewal_master.py | 4 +-
8 files changed, 274 insertions(+), 316 deletions(-)
diff --git a/install/tools/ipa-upgradeconfig b/install/tools/ipa-upgradeconfig
index adf6c8d847b931744cbfc4fcd14b6fbea55c26f5..e39523afe384c93a2f0915c36e577fcb7d4a448a 100644
--- a/install/tools/ipa-upgradeconfig
+++ b/install/tools/ipa-upgradeconfig
@@ -692,24 +692,23 @@ def certificate_renewal_update(ca):
# State not set, lets see if we are already configured
for request in requests:
nss_dir, nickname, ca_name, pre_command, post_command, profile = request
-criteria = (
-('cert_storage_location', nss_dir, certmonger.NPATH),
-('cert_nickname', nickname, None),
-('ca_name', ca_name, None),
-('template_profile', profile, None),
+criteria = dict(
+('cert-database', nss_dir),
+('cert-nickname', nickname),
+('template-profile', profile),
)
-request_id = certmonger.get_request_id(criteria)
+request_id = certmonger.get_request_id(**criteria)
if request_id is None:
break
-
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
On 08/19/2014 09:05 AM, David Kupka wrote:
> FreeIPA will use certmonger D-Bus API as discussed in this thread
> https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
>
> This change should prevent hard-to-reproduce bugs like
> https://fedorahosted.org/freeipa/ticket/4280
Thanks for this effort, the updated certmonger module looks much better! This
will help us get rid of the non-standard communication with certmonger.
Just couple initial comments from me by reading the code:
1) Testing needs fixed version of certmonger, right? This needs to be spelled
out right with the patch.
2) Description text in patches is cheap, do not be afraid to use it and
describe what you did and why. Link to the ticket is missing in the description
as well:
> Subject: [PATCH] Use certmonger D-Bus API instead of messing with its files.
>
> ---
3) get_request_id API:
> criteria = (
> -('cert_storage_location', dogtag_constants.ALIAS_DIR,
> - certmonger.NPATH),
> -('cert_nickname', nickname, None),
> +('cert_storage_location', dogtag_constants.ALIAS_DIR),
> +('cert_nickname', nickname),
> )
> request_id = certmonger.get_request_id(criteria)
Do we want to continue using the "criteria" object or should we rather switch
to normal function options? I.e. rather using
request_id = certmonger.get_request_id(cert_nickname=nickname,
cert_storage_location=dogtag_constants.ALIAS_DIR)
? It would look more consistent with other calls. I am just asking, not
insisting.
3) Starting function:
> +try:
> +ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
> skip_output=True)
> +except Exception, e:
> +root_logger.error('Failed to start certmonger: %s' % e)
> +raise e
I see 2 issues related to this code:
a) Do not call SYSTEMCTL directly. To be platform independent, rather use
services.knownservices.messagebus.start() that is overridable by someone else
porting to non-systemd platforms.
b) In this case, do not use "raise e", but just "raise" to keep the exception
stack trace intact for better debugging.
Both a) and b) should be fixed in other occasions and places.
4) Feel free to add yourself to Authors section of this module. You refactored
it greatly to earn it :-)
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
