leave default with current behavior - file 1.4 ticket to not have instance scripts

Ticket #48124 - Set configure option "yes" to eliminate instance specific scripts

Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1275763

attachment
0001-Ticket-47840-add-configure-option-to-disable-instanc.2.patch

The second attached patch is for the admin server, so that setup-ds-admin.pl has the correct behaviour using sbindir scripts rather than instdir scripts.

attachment
0001-Ticket-47840-fix-lib389-to-use-sbin-scripts.patch

The third patch fixes lib389 to use sbin scripts rather than per-instance scripts for start/stop.

Although each is small, may I ask a couple of things to be nicer for the reviewers and for the release process?

1) When attaching a file, could you add a brief attachment info? For instance, unless open 0001-Ticket-47840-add-configure-option-to-disable-instanc.2.patch​, it's hard to tell it is a patch for the Admin Server script and associate it with the comment 8. (Since you replaced the first patch with the same named one, the "second" patch becomes the first patch... :)

2) When you write a description in the comment section, could you put line breaks?
When opening a patch in a browser, the readability of the description would be higher if the line width is narrower and fixed.

3) 389-ds-base and 389-admin are different packages which have the separate target milestones. Could you please open another ticket for the Admin Server and attach 0001-Ticket-47840-add-configure-option-to-disable-instanc.2.patch to it?

Your patches themselves look good to me.

I have one question ... Although the installation was successful, I got this message. Do you have any idea?
restorecon: lstat(/usr/lib64/dirsrv/slapd-INSTNACE) failed: No such file or directory

Thanks!

Fixes restorecon message when instance scripts are disabled.
0001-Ticket-47840-add-configure-option-to-disable-instanc.patch

Added comment to attachment, I have reformatted the commit messages, and I will open the other tickets now.

The error you were seeing is because I didn't remove inst_dir from the updateSelinuxPolicy function for restorcon. I have corrected this now in the new patch. I've flagged you as the reviewer of all three, but I'll wait for you to approve before I push.

https://fedorahosted.org/389/ticket/48331
https://fedorahosted.org/389/ticket/48330

Tickets for admin and lib389.

Pushed to master
commit 20ebe8dd9cb9f17d0f09a9f0e5f65fc9a83d0b13

FIx regression from previous lib389 patch
0001-Ticket-47840-Fix-regression.patch

I only tested only single instance systems so I missed this. Looks good.

d343b74..83d5e37 master -> master
commit 83d5e3785c84914002441aba64e95ba8c340bfa9
Author: Mark Reynolds mreynolds@redhat.com
Date: Fri Nov 20 12:22:55 2015 -0500

Hi William,

Could you please change the default behaviour so that InstScriptsEnabled is not set, setup-ds.pl crates the instance dir with scripts? Currently, if it is not set, it does not generates them. As in the email discussion, we should not have made this big change in the minor update...

• my @dsdirs = qw(config_dir schema_dir log_dir lock_dir run_dir tmp_dir cert_dir db_dir ldif_dir bak_dir);
• if ($inf->{slapd}->{InstScriptsEnabled} eq "true") {
• @dsdirs = qw(inst_dir config_dir schema_dir log_dir lock_dir run_dir tmp_dir cert_dir db_dir ldif_dir bak_dir);
• }

Thanks!

Okay. I'll make this change, and we can queue up a ticket for 1.4.X to change this default. (Do we even have 1.4.X open yet ... ?)

commit 9b48d454c3089254a5cda39ceeab04ace444b8b4
To ssh://git.fedorahosted.org/git/389/ds.git
d7bbbf2..d01e436 master -> master

One line commit rules.

Hi William,

can we cherry-pick this commit to 1.3.5.x branches too? RHEL7.3 version of 389-ds-base will be a rebase from 1.3.5.

Thanks.

Replying to [comment:21 vashirov]:

Hi William,

can we cherry-pick this commit to 1.3.5.x branches too? RHEL7.3 version of 389-ds-base will be a rebase from 1.3.5.

Thanks.

Hi Viktor,
We haven't branched 1.3.5 yet. Let me respin the copr including this patch. Thanks!

attachment
0001-Ticket-47840-Add-InstScriptsEnabled-argument.patch

Add functionality to lib389.

Can we default to disabling the inst scripts?

Replying to [comment:25 firstyear]:

Can we default to disabling the inst scripts?

If I understand right, the default behaviour now is that setup-ds.pl creates the instance dir with scripts. Noriko reply:
"Could you please change the default behaviour so that InstScriptsEnabled? is not set, setup-ds.pl crates the instance dir with scripts? Currently, if it is not set, it does not generates them."

And we need to test three things: default behaviour without it, set to False, set to True. Because of that, I think it would be better to do not have it at all. If the DS would be changed to "disable it" by default, then it will affect lib389 and we will change the tests. Amita currently develops the test suite.

To ssh://git.fedorahosted.org/git/389/lib389.git
224e200..212518a master -> master
commit 212518aa9ae6783cb4f5cc5d2fdae72c35300c73
Author: Simon Pichugin spichugi@redhat.com
Date: Tue Dec 13 16:17:56 2016 +0100

Hi Amita,

• First of all, I think, we can describe the commit message better.

  In the subject, you need to put a short information about the commit.
  In our case, it is something like "Ticket 47840 - Add setup_ds test suite"

  And in the "Description", you need to have an info about what your commit has brought. Like: "Add two cases to test basic setup script functionality."
  There is no "Bug" or "Fix".

• Next. Please, use PyCharm ability to format your code (including imports). Code -> Reformat code

• You haven't removed an instance in "test_slapd_InstScriptsEnabled_True"

• It would be better to parametrize the test cases you have. It will remove the redundancy you have. Also, it is not difficult in your case. Just look at the guidelines, there is info.

• "# Create instance" - is a proper format for comments. "#remove the instance" - not.

A bit more to fix:

• Please, include imports in Code -> Reformat code

• In the create_instance(value), it would be better not to use 'value' name. It is better when variables are more descriptive about their nature.

• In patch, on line 45, it is better to use docstring for the function, not a comment.

• In patch, on line 55, you can change "dc=example,dc=com" to lib389 constant -> DEFAULT_SUFFIX

• According to PEP8, comments usually start with a capital letter.

• You assume that topology_st has value == ' ', but it may be changed in the future. It is better if your tests are isolated from that possibility.

• You can have one Setup (create_instance) and one tearDown part (standalone.delete()) for all cases without repeating in the each 'elif'

"""
76 + 3. As default behavior, the script directory should be created.
"""

This behaviour is incorrect. I think we don't create them by default.

Regardless, instance scripts will be removed in the future, so we should be ready for that.

We can simply test that "true vs false", we do not need to check default behaviours. We only need to assert behaviours work. So the parameters should be a True and False, as python boolean types, and you can have a single test that works like this.

You don't need to call standalone.delete, this is done in teardown.

@firstyear can you please merge the patch, if not any more review comments :) , thanks!

args_instance[SER_PORT] = 33333

There is a standalone port type you can use from constants somewhere that is better than 33333

Otherwise it all looks good.

@firstyear Please check.

Ack, this looks better. Do you need me to merge it?

To ssh://pagure.io/389-ds-base.git

434a92f..080f009  master -> master
commit 080f009e3c65cffa975484c6528119456cc5d8c7
Author: Amita Sharma <amsharma@redhat.com>
Date:   Wed Mar 15 19:08:09 2017 +0530

@spichugi Thanks

PFA for the setup test suit patch.

I think this looks okay to me. @spichugi ?

As per discussion on IRC.
Please, fix the commit message (include the mentioning of generage_ds_params change).
Also, we need to have 4 spaces every where, so replace 3 spaces with 4.
And your test fails now, because it checks for '/usr/lib64/dirsrv/slapd-standalone', but with the new generate_ds_params change you have - '/usr/lib64/dirsrv/slapd-standalone1'.

Fixed, please check.

Please check.

commit 60553e83225a96f80e21395671f41065905ac311
Author: Amita Sharma amsharma@redhat.com
Date: Wed Aug 23 19:13:31 2017 +0530

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 issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/1171

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.