This is looking very good. Just a question, why keeping 'ldif2db' or 'bak2db' ... subcommands ?
It could be an opportunity to rename the commands into 'import', 'export', 'backup', 'restore'.

@tbordaz I thought about this, and my reasoning is that admins will have enough to copy with when we change to dsctl.

People get very familiar and attached to things, names etc, and even though db2 and 2db names are not good, people know them. They have a familiarity with the concept and the idea of what it is. Even if our names are not perfect, people do not like "too much" change.

I agree with you, this is better to provide a smooth transition.

Does the framework support aliases of subcommand ? so that 'dsctl ldif2db...' would be supported as 'dsctl import...' ?

Anyway the fix is good and those things could land in a separated ticket

I haven't investigated command aliases yet. I think that we could do it, but that's a seperate issue like you say :)

Thanks for your review of this!

Looks good to me as well. I would only suggest to omit the parenthesis from the assert statements like we discussed the other day, just to avoid getting weird errors.

Apart from this I do have one more comment, but it's not directly related to this patch.

You use many times the status() method.
if self.status():
Every time I encounter this method I can't say what it does just from its name so I have to look for its definition, according to which it determines if an instance is running or not. I think that its name is a bit misleading, I would expect it to return a status code. Maybe a better name would be "is_running()"? I understand it might be difficult to change something like this at this point though.

Looks good to me as well. I would only suggest to omit the parenthesis from the assert statements like we discussed the other day, just to avoid getting weird errors.

Yep, I'll correct this.

Apart from this I do have one more comment, but it's not directly related to this patch.
You use many times the status() method.
if self.status():
Every time I encounter this method I can't say what it does just from its name so I have to look for its definition, according to which it determines if an instance is running or not. I think that its name is a bit misleading, I would expect it to return a status code. Maybe a better name would be "is_running()"? I understand it might be difficult to change something like this at this point though.

That's a good point. Sadly, the issue is that DirSrv is a bit of a kitchen sink, and has everything in it. We really could help remove this confusion by breaking out certain parts of dirsrv to other objects, but this requires us to run our full test suites to do it (which is a good thing to do anyway :) )

I don't think status is a confusing name, but I think it becomes confusing when it's surrounded by so much other code.

So what we really should look at is a DirSrv objectcleanup .

commit 4b97c75b04270f1554e548befbecdb3fa2f214f4
To ssh://git@pagure.io/lib389.git
46ef071..4b97c75 master -> master

commit 03a47457c97da79f2bce48e99051ef84d0179f26
To ssh://git@pagure.io/lib389.git
65499d4..03a4745 master -> master

One line rule fix for typo