Wouldn't it be better to use log.info() here? We consistently use it everywhere and it allows to set verbose, debug modes

I think the message is not verbose enough...

Probably, should take care of the lower case here

I think it will be nice to add a man file with detailed descriptions of commands that we run. So the user is aware of the decisions that we make for certutil commands. Like: the file names/paths, ciphers, etc.

I haven't tested it yet, but the rest of the code looks simple and easy to follow. Maybe @mhonek will have something to add...

We already have a security subcommand that does most of this. See /lib389/cli_conf/security.py

Perhaps you can extend the existing security CLI instead of a duplicate subcommand?

We already have a security subcommand that does most of this. See /lib389/cli_conf/security.py
Perhaps you can extend the existing security CLI instead of a duplicate subcommand?

dsconf vs dsctl - this needs local FS access to manipulate the nssdb, which is why it's not part of the security command ....

I think it will be nice to add a man file with detailed descriptions of commands that we run. So the user is aware of the decisions that we make for certutil commands. Like: the file names/paths, ciphers, etc.

What do you mean here? Like improving the help section? Or by explaining what options we choose?

Generally the idea here is we encapsulate best practices and then over time we can always improve and upgrade these decisiosns so users don't have to.

We already have a security subcommand that does most of this. See /lib389/cli_conf/security.py
Perhaps you can extend the existing security CLI instead of a duplicate subcommand?

dsconf vs dsctl - this needs local FS access to manipulate the nssdb, which is why it's not part of the security command ....

Thought it was dsconf, ugh. I'll shut up now :-p

@spichugi So part of the question for you was about how we should structure and display the certificate data - that's what I'd really like some feedback on.

We already have a security subcommand that does most of this. See /lib389/cli_conf/security.py
Perhaps you can extend the existing security CLI instead of a duplicate subcommand?
dsconf vs dsctl - this needs local FS access to manipulate the nssdb, which is why it's not part of the security command ....

Thought it was dsconf, ugh. I'll shut up now :-p

All good, easy mistake to make. Please don't shut up, I need the comments!

I think it will be nice to add a man file with detailed descriptions of commands that we run. So the user is aware of the decisions that we make for certutil commands. Like: the file names/paths, ciphers, etc.

What do you mean here? Like improving the help section? Or by explaining what options we choose?
Generally the idea here is we encapsulate best practices and then over time we can always improve and upgrade these decisiosns so users don't have to.

Yeah, basically, what I meant is to describe in more detail and explicitly these decisions in the help section.
So in the result, they are displayed in man dsctl info too.

Probably, more of a nitpick but still. All over the CLI we use 'dash' approach. So maybe we can rename the commands to remove-cert in this tool too?

Probably, more of a nitpick but still. All over the CLI we use 'dash' approach. So maybe we can rename the commands to remove-cert in this tool too?

Yes please make this conversion, nice catch @spichugi !

@spichugi So part of the question for you was about how we should structure and display the certificate data - that's what I'd really like some feedback on.

I think the main thing we should keep in mind - is that it should be simple (we already have 'certutil' for complex and detailed operations). And I think your structure is good in that term. :)

Another point to remember - is that the user should be able to pass the displayed certificates through pipes. So it should be straight forward there without any not needed strings.

The rest, I think, looks good and fine-grained enough to cover most of the operations.
But I've always done only basic certutil stuff (generate cert, sign it, import, etc.)...

And to check if we don't break any other, more rare cases that we can encounter while managing DS - I think, we need @mhonek here :)

So I've added thumbs as I checked off each comment as I worked on it. I agree with your comment that complex and detailed things are for certutil. This is just for some simple transforms an "basic" day to day. It's mainly because I'm actually too lazy to use certutil, and if IM too lazy, imagine how our customers feel .....

Anyway, I think the piping may be an advanced case - I'll leave that to certutil. But you are right, we should keep this simple, so I think I'll make the display either just:

• for CA listing show nick names + expiry date + subject
• For details on a specific cert we effectively just wrap certutil -L -d path -n nickname, to make it a bit easier to display details without reinventing this ourselves.

As an aside, the positional args can NOT be renamed from say cert_path to cert-path as cert-ptah is not a valid python variable ....

As an aside, the positional args can NOT be renamed from say cert_path to cert-path as cert-ptah is not a valid python variable ....

Yep, what I meant is just the parsers, not arguments, of course.
Maybe I misread and the parsers were okay... I can't tell now because of the rebase.
And sorry for the noise if it's on me. :D

The rest looks good but I still haven't tested it.
Also, I'll leave for others to check too because it's important to agree about the structure and the made decisions of such tool together.

Yep. It would be good for you to test this I think before giving the yay/nay. @mreynolds do you have thoughts here too?

poke @spichugi and @mreynolds :)

Everyone just seems so busy lately :)

Review reminder :)

Yeah works nicely, ack

Awesome! Thanks I'll merge this soon then.

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3703

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.