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.

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 :-/

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.

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.

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.

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

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

Thanks, I agree that more subparsers was correct :)

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?

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. @spichugi where are we storing the CLI tests now? I know there's been concern about where we are putting things. Thanks!

I'll open a new ticket and work on it next before the UI part. @spichugi 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 :)

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

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!

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

I'll open a new ticket and work on it next before the UI part. @spichugi 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

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

Great! LGTM, ack!

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-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/3090

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.