PR - Ticket 50619 - extend commands to have more modify options

[389-ds-bot]

Cloned from Pagure Pull-Request: https://pagure.io/389-ds-base/pull-request/50624

• Created at 2019-09-25 06:58:31 by firstyear (@Firstyear)
• Merged at 2019-10-02 01:27:39

---

Bug Description: Extend dsidm to support modifying more types of
entries.

Fix Description: Can now modify groups, posixgroup, ou and others
from the cli without an ldifmodify

Resolves: #3674

Author: William Brown william@blackhats.net.au

Review by: ???

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-09-25 13:43:44

Looks good!
One small nitpick - the error output has the "pythonic leftovers":

$ sudo dsidm localhost -b dc=example,dc=com group modify new add:description:qwe
Error: {'desc': 'Type or value exists'}
$ sudo dsidm localhost -b dc=example,dc=com group modify new delete:description:qwe
Error: {'desc': 'No such attribute'}

It will be nice to get the 'desc' value and use it in the output.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-09-26 04:58:35

I'm not actually sure I can fix this. This is part of how the error presentation code is dsctl/dsconf/dsidm works.

If you look in dsidm about line 126, you'll see a try/except block, and in the except we do:

log.debug(e, exc_info=True)
log.error("Error: %s" % str(e))

So we attempt to string-ify the exception. The issue is that most exceptions are fine for this:

dirsrv@ldapkdc:/home/william/development/389ds/ds/src/lib389> PYTHONPATH=./ ./cli/dsidm localhost group modify demo add:description:test
Error: No object exists given the filter criteria demo

So that prints a lovely message. If we look at -v it's from ldap.NO_SUCH_OBJECT: No object exists given the filter criteria demo

But ldap.TYPE_OR_VALUE_EXISTS has a dict instead, and that's why it formats weirdly.

So I think this is a fault of how str/unicode on ldap.TYPE_OR_VALUE_EXISTS works, not a fault of my patch :)

I'll let you comment on this first before I push

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-09-26 18:26:57

So I think this is a fault of how str/unicode on ldap.TYPE_OR_VALUE_EXISTS works, not a fault of my patch :)
I'll let you comment on this first before I push

Yeah, but if we leave it like this in the CLI, there will be complains for sure because it doesn't look "clean".
What I propose is to have exception processing at some point in the CLI.
We can get the description from e.args[0]['desc']. Yeah, it is not ideal but it will fix the issue.

So either at some point around log.error("Error: %s" % str(e)), either somewhere areound _generic_modify.
What do you think?

Also, it can be another PR. It's up to you. I agree that it is not this PR's fault.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-09-27 01:47:19

No we can't. Because not everything has e.args[0][desc]. It's specific to this one ldap error, and we'll trigger an indexerror on everything else that uses e.message. It's a bug in pyldap's str/unicode for this error type. If we are going to fix it, we need to fix it in pyldap, not in our code to make it work correctly.

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-09-27 09:11:48

My point is that we can fix it at least for the cases where e.args[0][desc] exists.
As I said previously, there will be complains from users/customers.

So we can have some workaround which can be removed later (we can add the link to python-ldap issue in a comment for tracking purposes).
The option - check if e.args[0][desc] exists and print the content - str(e) otherwise - will cover most of the cases (I've never seen an ldap.ERROR which doesn't have it, actually). And for what's left - we can say our users that we've done everything and the rest of the solution is dependent on python-ldap.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-09-28 04:23:54

There is only one case where e.args[0][desc] exists and it's that one ldap error. I'm really hesitant to write work arounds because then we'll never fix the original problem. We just mask over it.

I think we should accept this patch as is, because the error could happen in many other places, and you should report a bug with str/unicode python ldap to fix this.

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-09-30 12:19:26

There is only one case where e.args[0][desc] exists and it's that one ldap error. I'm really hesitant to write work arounds because then we'll never fix the original problem. We just mask over it.
I think we should accept this patch as is, because the error could happen in many other places, and you should report a bug with str/unicode python ldap to fix this.

IIRC, every ldap.LDAPError returns the e.args[0]['desc']...
For example, @vashirov fixed here - https://pagure.io/389-ds-base/c/af4631f

If we won't do the workaround I proposed, the users will eventually (and I think pretty soon) complain that WebUI and CLI have these weird formatted errors. And it will take a while because the writing, and merging, and waiting for the python-ldap build will take a few months.

I've done the same with another upstream issue - I've masked the problem and put the comment with the link to the upstream issue. When it was resolved - I changed the code back.

Maybe @mreynolds389 can have a fresh look on the issue and share his opinion too... :)

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-10-01 02:49:49

@droideck Should this be a seperate issue then?

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-10-01 10:01:30

Yep, as I mentioned - #3679#comment-97740

Also, it can be another PR. It's up to you. I agree that it is not this PR's fault.

You have my ack this one if so :)

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-10-02 01:25:21

Did you want to open the separate issue then?

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-10-02 01:26:36

rebased onto 82536959b9f28fecfd63766c0788515cfce964dd

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-10-02 01:27:02

rebased onto 205778f

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-10-02 01:27:39

Pull-Request has been merged by Firstyear

[389-ds-bot]

Patch
50624.patch