Typo - 'numben'

It is really a small nitpick but in Python catching more than you need is very bad practice...

In that case, probably, nothing bad will happen now, but if someone will change the code in the future and 'val' will become an integer type, then 'int(val)' will return TypeError exception. It will lead to misleading situation.

Could you please change it to 'except ValueError'?

The same here and in the other lines with 'try-except' checks.
The docs say that we should catch 'except KeyError' - https://docs.python.org/3.6/library/pwd.html

We can take most of the options from defaults.inf file.
You can take an example from ../options.py module.
Basically, it works like this:

from lib389.paths import Paths
ds_paths = Paths()
print(ds_paths.log_dir)

You get the same value - /var/log/dirsrv/slapd-{instance_name}

We have an issue here during the input...

Enter Directory Manager Password: omg_you_can_see_my_password

Confirm Directory Manager Password: omg_you_can_see_my_password

After you have the instance name, you can set it with 'format' function in the existing 'slapd' dict values:

for key, value in slapd.items():
    slapd[key] = value.format(instance_name=slapd['instance_name'])

Also, the commit header has a typo too - 'interative'

@spichugi Thanks for the review! I meant to change the rootdn password, but forgot. I wanted to get this out for review before being out "today", so I'll get this stuff wrapped up tomorrow

@spichugi - changes applied and tested

I am not sure if you've done it intentionally or not, but you have one more 'except' here

I see that we don't have the password strength check here (as we have in setup-ds.py)
But I think we can add it later with the following patches.

Thank you! Besides the small issue, I mentioned the code looks good to me! Ack

P.S. sorry, typo in the previous comment - (as we have in setup-ds.pl)

I know it's been merged but I'll do a review of this too for you.

You might only want to install the signal hanler when verbose is false. This way a stuck install during debug still traces.

I'd rather this be an argument "--interactive" rather than the abscence of an argument. Because now you have to handle the state of "no file and other options provided" like generating the example.

Everything should be explicit and requested, so that there is no ambiguous states.

Don't validate this here! Leave it to the validation step later. You only need to validate that it's a number, but the open state of the port is checked later and should remain there.

You use this pattern a lot. Perhaps you should make a function like "get_interactive_answer" that takes a prompt, and the set of "valid" and "invalid" responses, then loops for you?

Do not perform validation of the user here - it's done in the verification code. The whole point of dscreate is to SHARE validation code for API, interactive and from file installs.

Please remove this. Allowing install prefixs breaks SELinux and should not be "interactively" offered so easily for such a dangerous option.

Don't validate here again ...

Don't check this here again, this is validated elsewhere.

You should allow the creation of NO backends, as this is also a valid config (albeit edge case)

Choice for example entries?

Don't add self.verbose flags. Adjust the level of info from the logger instead, or the level these messages are set at.

Don't pipe this to devnull, then useful debug info is lost in an error condition

Hey there. I've commented on this. I have a lot of concerns about your approach to this code, especially with the inline validhation. There are already extensive validations of input data, and they don't need to be "inline" in the cli. They are executed after. I also think that some small changes to make an interactive prompt would make your life a lot easier code wise. Finally, I think that the interactive installer asks too many risk questions. Prefixs, custom usenames and more. These are all fast ways to break your install out of box. Although the IDEA of customisation sounds lovely, the reality is no one does it, and it causes pain. Please remove these options (user to run as, group to run as, prefix) and leave them as "expert" options for the file install (even then I want to remove them).

Thanks,

@firstyear thanks for the review! I'll work on changing some of this, but I have to respectfully disagree with your concerns about the validation I am doing. Yes, it is duplicate, but that's partially due to the fact that dscreate was designed only work with files, and not an interactive component. And it's the interactive component that is the focus of this ticket. If I remove the validation then the interactive installer becomes less interactive. If you enter an invalid value you should have the option to correct it - not find out at the very end you entered bad data and have to start all over again. From the "interactive perspective", validating after it's too late to change anything when you could have corrected it is a bug/design flaw. Perhaps in the next major release we can rewrite dscreate and better mesh the file/interactive logic and remove this duplicate validation, but I am not going to make the interactive installer dumb and rigid just because there is duplicate code and design limitations.

Besides that I agree with everything else. I didn't really want to add the prefix, but I know a lot of people use it, and the user and group isn't really necessary, etc. All of this is fine to leave in the file install.

So next week sometime I will work on revising & removing the install options and address your other concerns. Thanks again!

The words in some questions in the interactive installer start with a capital letter and in some questions only the first word does. For consistency reasons, all should be the same. Preferably use sentence case in all questions.

Can we use "name of the instance" or something similar here? In the past, the docs always talked about "instance name". It might be clearer.

And some of the questions are in instruction format (Enter ...) and some just use a label (e. g. "Installation prefix"). For consistency, I would switch to one format, preferably, non-instruction format.

I suggest using "interactive" (without --), to be consistent with the current arguments "install" and "create-template".

I suggest using "interactive" (without --), to be consistent with the current arguments "install" and "create-template".

If we do this, then it makes sense to go back to "fromfile" instead of "install" as @firstyear originally intended.

I suggest using "interactive" (without --), to be consistent with the current arguments "install" and "create-template".

If we do this, then it makes sense to go back to "fromfile" instead of "install" as @firstyear originally intended.

I agree.

Hey mate. Always happy to review (and always happy to agree and disagree sometimes ;) )

I want to break down the validation part to help explain a bit better. The issue is that validation in seperate places can cause confusion and odd behaviours.

Because there are now two validation sources, they may diverge or check different things. This means that the requirements for a file install may diverge from an interactive one. It's also possibly a source of confusion to why such a discrepancy exists.

Now today, maybe that's not a problem. Today there is not much in the interactive part. But you know, over a few years, an extra check, extra check there. Now there are two divergent validation paths.

Rather than having such a discrepancy, I'd rather it be in a single place as a source of truth. You can do dry runs, add features to "check all the values" rather than erroring on the first, etc. You know exactly where a check goes (one place) rather than now having two places. There are ways to improve the validation to make it better integrated to the file and interactive process (collect all errors rather than fail fast). I think this is a better long term approach for our sanity :)

It's both good code hygiene but a better long term user experience, and a better testing and qe experience. Now validation can be tested as an API instead of invoking the CLI tools each time. Testing was a huge design part of this framework, and that's why the validation code is seperated out so it can be tested in isolation.

Anyway, I'm glad we agree on the other parts. I really do want the installer interactively to be simpler and "less prone" to mistakes. I would assert our current success as a project has been attention to "it just works" and "correct out of box", so sometimes removing choice is better for us (less bugs, less surface area to test etc).

Thanks mate. Take care : )

Hey @firstyear, actually I agree with you about having two different validation code paths, and I did say we should address that in the next release ( to not have duplicate checks). I would rewrite it now, but we are out of time, and there are bigger fishes to fry right now to get the CLI/UI ready for the next release.

The changes I recently made are here though:

https://pagure.io/389-ds-base/pull-request/49838

Thanks,
Mark

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

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.