search

389ds / 389-ds-base

The enterprise-class Open Source LDAP server for Linux
https://www.port389.org/
Other
216 stars 94 forks source link

PR - Ticket 49994 - Add backend features to CLI #3090

Closed 389-ds-bot closed 4 years ago

389-ds-bot commented 4 years ago

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

Description:

Added backend features (chaining, db, indexes, vlv, attr encryption, and monitor to the CLI.

Addressed:

Resolves: #1940 - that prevented VLV search/index entries from being updated. Resolves: #3084 - dsconf prints out to stderr

Also updated jstree js file.

Resolves: #3053

Reviewed by: ?

389-ds-bot commented 4 years ago

Comment from spichugi (@droideck) at 2018-11-19 12:07:31

I think it makes more sense to have subparsers like this:

dsconf instance backend suffix get 
dsconf instance backend suffix backup

instead of 

dsconf instance backend suffix-get
dsconf instance backend suffix-backup

The same for other CLI commands. IMO, it looks more natural for a user.

I'll be testing and checking other details more today, but it looks very good out of the first look.

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2018-11-19 14:38:52

I think it makes more sense to have subparsers like this: dsconf instance backend suffix get dsconf instance backend suffix backup

instead of dsconf instance backend suffix-get dsconf instance backend suffix-backup

But "backend" and "suffix" are redundant. Plus there is no "suffix-backup" argument :-)

The main reason I added "suffix-" to a few of the options was for easier usage of the tool (since there are so many options). Look at the usage below I think you will see what I was trying to achieve. Notice how all the options are grouped together by the prefix:

[root@localhost cli]# ./dsconf localhost backend 
attr-encrypt     export           import           index-edit       index-reindex    suffix-create    suffix-get-dn    suffix-set       vlv-del-index    vlv-get
db-config-get    -h               index-add        index-get        monitor          suffix-delete    suffix-get-subs  vlv-add-index    vlv-del-search   vlv-index
db-config-set    --help           index-del        index-list       monitor-suffix   suffix-get       suffix-list      vlv-add-search   vlv-edit-search  vlv-list

I thought this was actually very nicely organized and logical :-/

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2018-11-19 14:42:11

Also if we keep adding subparsers I feel the tool will get too complicated. I think having all the options like I do above is easier for discovery of usage. Make sense? I not fully opposed to changing it, but I like the way I have it, and i think it would be easier for admin to consume the tool this way then to keep burying options under additional subsparsers.

389-ds-bot commented 4 years ago

Comment from spichugi (@droideck) at 2018-11-19 15:28:34

Exactly because of the discoverability, I think, it is better with more subparsers.

So when admin tries to find how to list a suffix, he will:

1. Type - [root@localhost cli]# ./dsconf localhost backend - Press tab
export           import           index       suffix       vlv        monitor
2. Select 'suffix' option - Press tab
list     create      delete      get      get-dn      set
3. Select 'list'

Instead of the whole selection of the options for the backend:

[root@localhost cli]# ./dsconf localhost backend 
attr-encrypt     export           import           index-edit       index-reindex    suffix-create    suffix-get-dn    suffix-set       vlv-del-index    vlv-get
db-config-get    -h               index-add        index-get        monitor          suffix-delete    suffix-get-subs  vlv-add-index    vlv-del-search   vlv-index
db-config-set    --help           index-del        index-list       monitor-suffix   suffix-get       suffix-list      vlv-add-search   vlv-edit-search  vlv-list

But we need to make sure to add the 'tab' completion support. Shouldn't be hard though. I thought to add it soon after the CLI work is finished.

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2018-11-19 15:28:39

Actually.... :-) After thinking about this more it would make sense to add additional subparsers. Regardless there are going to be a lot of options. So I will break out: suffix. index, and vlv into their own subparsers.

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2018-11-19 15:56:38

rebased onto b94240394be75fc01ae4b3625d2595457f60bffd

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2018-11-19 17:16:56

Exactly because of the discoverability, I think, it is better with more subparsers.

I agree with you, and I did rebase the PR...

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2018-11-19 22:19:59

Thanks, I agree that more subparsers was correct :)

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2018-11-19 22:22:22

My only concern here is the lack of tests. lib389 supports the test framework for the cli and the objects, and there is a lot of code here. I know you are working hard to push over this feature to a deadline, but I also know if we don't add tests now/soon, we never will. Can we either add tests now, or open a ticket to add them soon after the deadline is over?

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2018-11-19 22:50:37

My only concern here is the lack of tests. lib389 supports the test framework for the cli and the objects, and there is a lot of code here. I know you are working hard to push over this feature to a deadline, but I also know if we don't add tests now/soon, we never will. Can we either add tests now, or open a ticket to add them soon after the deadline is over?

I'll open a new ticket and work on it next before the UI part. @droideck where are we storing the CLI tests now? I know there's been concern about where we are putting things. Thanks!

389-ds-bot commented 4 years ago

Comment from spichugi (@droideck) at 2018-11-20 00:19:53

I'll open a new ticket and work on it next before the UI part. @droideck where are we storing the CLI tests now? I know there's been concern about where we are putting things. Thanks!

For now, it is just here - https://pagure.io/389-ds-base/blob/master/f/src/lib389/lib389/tests/cli But I think we will move it to something like dirsrvtests/tests/suites/cli/ The final decision belongs to @vashirov though :)

389-ds-bot commented 4 years ago

Comment from spichugi (@droideck) at 2018-11-20 12:05:04

According to https://www.python.org/dev/peps/pep-0008/#class-names it should be without underscore... Could we have it like ChainingConfig? And the rest of the classes in the file too

389-ds-bot commented 4 years ago

Comment from spichugi (@droideck) at 2018-11-20 12:10:08

Okay... I've checked the rest of the code and I've done some testing:

In the UI, it fails after the instance installation with the following error:

usage: dsconf instance backend [-h]
                           {suffix,index,vlv,attr-encrypt,monitor,import,export}
                           ...
dsconf instance backend: error: invalid choice: 'list' (choose from 'suffix', 'index', 'vlv', 'attr-encrypt', 'monitor', 'import', 'export')

Another strange whing has happend... I can't list suffixes... Do I do something wrong?

# ./src/lib389/cli/dsconf -D "cn=Directory manager" local1 backend suffix list
usage: dsconf instance backend suffix [-h] {get,set} ...
dsconf instance backend suffix: error: invalid choice: 'list' (choose from 'get', 'set')

And could you please add docstrings to lib389 libraries (not CLI though, just to modules like src/lib389/lib389/backend.py, src/lib389/lib389/chaining.py)? We should have it anyway and it's easier for the person who added the code. :)

The rest looks good. Thanks!

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2018-11-20 22:40:10

rebased onto 4a76e693acd5f867da38e4487bac456533e37830

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2018-11-20 22:41:15

@droideck - fixed UI suffix list command, fix chaining class names, and added docs to lib389 classes. Please review...

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2018-11-21 17:23:53

rebased onto f2b06b217d6b6b9ebadb82ff785138ad148cc546

389-ds-bot commented 4 years ago

Comment from vashirov (@vashirov) at 2018-11-21 23:26:33

I'll open a new ticket and work on it next before the UI part. @droideck where are we storing the CLI tests now? I know there's been concern about where we are putting things. Thanks!

For now, it is just here - https://pagure.io/389-ds-base/blob/master/f/src/lib389/lib389/tests/cli But I think we will move it to something like dirsrvtests/tests/suites/cli/ The final decision belongs to @vashirov though :)

For integration tests we have CLU test suite: https://pagure.io/389-ds-base/blob/master/f/dirsrvtests/tests/suites/clu/ For lib389 unit tests we should put them under lib389/tests

389-ds-bot commented 4 years ago

Comment from spichugi (@droideck) at 2018-11-22 00:34:35

@droideck - fixed UI suffix list command, fix chaining class names, and added docs to lib389 classes. Please review...

Great! LGTM, ack!

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2018-11-23 15:40:01

rebased onto 89886ba4ee67058bb3448d26f772b66767097f0e

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2018-11-23 15:40:37

Pull-Request has been merged by mreynolds389

389-ds-bot commented 4 years ago

Comment from vashirov (@vashirov) at 2018-12-05 12:03:21

Sorry, I'm late to the party.

suites/attr_encryption/attr_encryption_test.py is failing after this backend.py update: https://fedorapeople.org/groups/389ds/ci/nightly/2018/12/05/report-389-ds-base-1.4.0.19-20181205git5eab3b5.fc29.x86_64.html

389-ds-bot commented 4 years ago

Patch 50031.patch