389-ds-base

Clone
Source Code
GIT

#50668 Ticket 50667 - dsctl -l did not respect PREFIX
Closed 3 years ago by spichugi. Opened 4 years ago by firstyear.
firstyear/389-ds-base 50667-fix-prefix  into  master

file modified
+2 -2 
@@ -47,8 +47,8 @@ 
 

          default=False, action='store_true'
 

      )
 

  

- parser.add_argument('--remove-all', nargs="?", default=False, const=None,
 

-         help="Remove all instances of Directory Server (you can also provide an optional directory prefix for this argument)",
 

+ parser.add_argument('--remove-all', default=False, action='store_true',
 

+         help=argparse.SUPPRESS
 

      )
 

  

  subparsers = parser.add_subparsers(help="action")

file modified
+1 -1 
@@ -127,7 +127,7 @@ 
 

      """Remove all instances - clean sweep!
 

      """
 

  

-     inst_names = get_instance_list(args.remove_all)
 

+     inst_names = get_instance_list()
 

      if len(inst_names) > 0:
 

          answer = input("Are you sure you want to remove all the Directory Server instances?  Enter \"Yes\" to continue: ")
 

          if answer != 'Yes':

file modified
+6 -2 
@@ -1252,9 +1252,10 @@ 
 

          return ("ldap://{}:{}".format(host, port), None)
 

  

  

- def get_instance_list(prefix=None):
 

+ def get_instance_list():
 

      # List all server instances
 

-     conf_dir = (prefix or "") + "/etc/dirsrv/"
 

+     paths = Paths()
 

+     conf_dir = os.path.join(paths.sysconf_dir, 'dirsrv')
 

      insts = []
 

      try:
 

          for inst in os.listdir(conf_dir):
 
@@ -1262,6 +1263,9 @@ 
 

                  insts.append(inst)
 

      except OSError as e:
 

          log.error("Failed to check directory: {} - {}".format(conf_dir, str(e)))
 

+     except IOError as e:
 

+         log.error(e)
 

+         log.error("Perhaps you need to be a different user?")
 

      insts.sort()
 

      return insts

Bug Description: dsctl list was not coded to allow
using the paths module.

Fix Description: Change to the paths module to allow
better and consistent CLI handling.

https://pagure.io/389-ds-base/issue/50667

Author: William Brown william@blackhats.net.au

Review by: ???

@spichugi and @vashirov if you'd like to review this please :)

I am not sure if it's by design but now it works like this:
dsctl --remove-all ANY_TEXT

I think it should be like this:
dsctl --remove-all

P.S. can't it be dsctl remove-all by the way?

I am not sure if it's by design but now it works like this:
dsctl --remove-all ANY_TEXT
I think it should be like this:
dsctl --remove-all
P.S. can't it be dsctl remove-all by the way?

I added the initial "--removal-all" feature, but the more I use it the more I don't like it. Any dsctl subcommand can append --remove-all (that could be very bad), and it's just not consistent with the usage. I'd totally be on board changing it, but not sure how. dsctl (and the other cli tools) want you to specify a instance, but if its a operation that impacts all instances it just doesn't fit well. I want to keep the "feature", but not sure what is the best way to implement it more cleanly. Anyway, just trying to start a conversation :-)

I think I get why you have info here instead of error.
But just clarify, is it intentional?
I am not hard on this but it feels a bit off... Like the wrong user is still an error, right?

I am not sure if it's by design but now it works like this:
dsctl --remove-all ANY_TEXT
I think it should be like this:
dsctl --remove-all
P.S. can't it be dsctl remove-all by the way?

I added the initial "--removal-all" feature, but the more I use it the more I don't like it. Any dsctl subcommand can append --remove-all (that could be very bad), and it's just not consistent with the usage. I'd totally be on board changing it, but not sure how. dsctl (and the other cli tools) want you to specify a instance, but if its a operation that impacts all instances it just doesn't fit well. I want to keep the "feature", but not sure what is the best way to implement it more cleanly. Anyway, just trying to start a conversation :-)

Agree.
I think it can be a separate subcommand like dsconf remove-all. I just need to think if it's possible to have it with the current command layout and how (I mean the fact that it should be dsctl inst ...)

I am not sure if it's by design but now it works like this:
dsctl --remove-all ANY_TEXT
I think it should be like this:
dsctl --remove-all

But ... that's what it does? The change was from:

dsctl --remove-all /opt/dirsrv
to
PREFIX=/opt/dirsrv dsctl --remove-all

P.S. can't it be dsctl remove-all by the way?

No because then we have to parse instance name into a possible command? That's why dscreat is seperate from dsctl as not to polute the first positional argument.

I think I get why you have info here instead of error.
But just clarify, is it intentional?
I am not hard on this but it feels a bit off... Like the wrong user is still an error, right?

I think it should be an error, yes. My mistake :)

rebased onto dd31f8c6040c610d9ede584ebf57e09f96c709dd
4 years ago

Updated to fix the info to error.

But ... that's what it does? The change was from:
dsctl --remove-all /opt/dirsrv
to
PREFIX=/opt/dirsrv dsctl --remove-all

Yeah, but that doesn't work for me...

[root@host ds]# PREFIX=/opt/dirsrv dsctl --remove-all
usage: dsctl [-h] [-v] [-j] [-l] [--remove-all REMOVE_ALL]
             [instance]
         {restart,start,stop,status,remove,db2index,db2bak,db2ldif,dbverify,bak2db,ldif2db,backups,ldifs}
         ...
dsctl: error: argument --remove-all: expected one argument

But this works:

[root@host-10-0-137-126 result]# dsctl --remove-all any_word
Are you sure you want to remove all the Directory Server instances?  Enter "Yes" to continue:     Yes
Removing instance: slapd-localhost
Removing instance: slapd-master1
All instances have been successfully removed

That was the thing I was talking about. Sorry if it wasn't clear...
You probably want to make remove-all option as a flag (with action='store_true')

P.S. can't it be dsctl remove-all by the way?

No because then we have to parse instance name into a possible command? That's why dscreat is seperate from dsctl as not to polute the first positional argument.

I wonder if we can separate the functionality to dsremove... But that's too late probably.

And I am still not 100% sure that worths it at all because the action remove all instances is VERY rare thing to do in real life.

No because then we have to parse instance name into a possible command? That's why dscreat is seperate from dsctl as not to polute the first positional argument.

I wonder if we can separate the functionality to dsremove... But that's too late probably.
And I am still not 100% sure that worths it at all because the action remove all instances is VERY rare thing to do in real life.

Well it's not too late to add a new tool, but creating a new tool "dsremove" that only performs a single task is silly :-). I wonder if we should add it to dscreate? dscreate remove-all ??

Well it's not too late to add a new tool, but creating a new tool "dsremove" that only performs a single task is silly :-). I wonder if we should add it to dscreate? dscreate remove-all ??

dsremove can be the same as the old tool (remove one instance or remove-all):

remove-ds.pl --help
Usage: /usr/sbin/remove-ds.pl [-a] [-f] [-d -d ... -d] -i instance

 Opts: -a            - remove all
            -f            - force removal
            -i instance   - instance name to remove (e.g. - slapd-example)
            -d            - turn on debugging output

As for adding the remove option to dscreate, wouldn't it be too confusing? Like, it has a create word in the name which defines its purpose.

Well, I never intended for ds to have a "remove all" option ... it was always meant to be:

dsctl <instance> remove --doit

And to remove all you just iterate over your instances.

So I don't think we need a dsremove at all, because it would only serve the remove all case .Which really is rare anyway ....

Maybe I miss something, but what about my comment that
PREFIX=/opt/dirsrv dsctl --remove-all doesn't work?

There is the error I get:
https://pagure.io/389-ds-base/pull-request/50668#comment-103297

It's expecting a instance identifier. This is because dsctl was always intended and structure to work with a single instance, and never "all instances" as a collection. that's why it's complaining that an argument is missing.

I'd honestly just advocate to remove --remove-all, because to "fix" this to have instance as "required" but also "optional" seems like a mess ....

IMHO -l option list the available instances, so for an admin this is immediate to script a loop to remove the instance one by one. If remove-all is complex to implement or goes against the philosophy of dsctl, I agree with @firstyear it can be drop. Just my 2cts

It's expecting a instance identifier. This is because dsctl was always intended and structure to work with a single instance, and never "all instances" as a collection. that's why it's complaining that an argument is missing.
I'd honestly just advocate to remove --remove-all, because to "fix" this to have instance as "required" but also "optional" seems like a mess ....

I think it is complaining about

dsctl: error: argument --remove-all: expected one argument

And in the help it says [--remove-all REMOVE_ALL]

If I do it like this:

parser.add_argument('--remove-all', default=False, action='store_true',
        help="Remove all instances of Directory Server (you can also provide an optional directory 
prefix for this argument)",
    )

It seems to fix the issue. And now it works (and without instance identifier).

$ sudo dsctl --remove-all
Are you sure you want to remove all the Directory Server instances?  Enter "Yes" to continue:

So, I'd suggest Simon's action='store_true', and also help=argparse.SUPPRESS to hide it since it's not a very safe nor used option.

That's because whet you do "dsctl --remove-all REMOVEALL" the parser is reading REMOVEALL as an instance name, and then the --remove-all flag now exists setting the value to true.

I agree with mhonek here, we should supress this option, and really, it should be removed.

Follow up - this isn't really part of this issue, so I think we should open a new issue for suppressing and removing remove all?

@spichugi @mhonek Can we merge this and fix the remove all seperately?

I guess we can. But wait for @spichugi, too, please.

rebased onto 84abf328341bce2b2b386ec3aae2b260e4799b6b
4 years ago

This supresses remove-all so that it's hidden (it's the @vashirov only special now).

Anything else @spichugi ?

Could you please add action='store_true', to parser.add_argument('--remove-all', default=False, as I mentioned above?

The rest looks good to me.

rebased onto b133a74
4 years ago

Updated as requested :)

LGTM! Ack

Pull-Request has been merged by firstyear
4 years ago

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

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.

Pull-Request has been closed by spichugi
3 years ago
Metadata
None
No Tags
Changes Summary 3
+2 -2
file changed
src/lib389/cli/dsctl
+1 -1
file changed
src/lib389/lib389/cli_ctl/instance.py
+6 -2
file changed
src/lib389/lib389/utils.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.