PR - Ticket 49712 - lib389 CLI tools should return a result code on failures

[389-ds-bot]

Cloned from Pagure Pull-Request: https://pagure.io/389-ds-base/pull-request/49776

• Created at 2018-06-12 20:42:00 by mreynolds (@mreynolds389)
• Merged at 2018-06-14 02:09:47

---

Description: I've also included the work for 49775 in this patch since
there was a lot of overldap.

          For dsctl functions we need to check for True and False in
          order to detect an error.  For dsconf & dsidm we need to
          catch exceptions.  Once an error is detected we return error
          code (1).

          The changes for 49775 was to use the default archive directory
          if one was not specified to db2bak, and use the default ldif
          location for db2ldif.  This how the old tools worked, no
          reason not to carry over this convenience.  Also the format
          used for the file name (Instance name + Date/Time) is the same
          as the old cli tools.

          Also did some pep8 cleanup.

Resolves: #2771

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-06-12 20:59:40

rebased onto e407dc250aca10d91ab9e415eddb45cd4a5c1777

[389-ds-bot]

Comment from spichugi (@droideck) at 2018-06-13 11:09:16

If I understand correctly, by documentation, return_value = PR_Rename(directory, dir_bak); returns either PR_SUCCESS (0), either PR_FAILURE (-1). So the line you've added seems redundant to me... Or do I miss something?

[389-ds-bot]

Comment from spichugi (@droideck) at 2018-06-13 12:14:50

I think we should print the error to debug so we don't lose the information.
You can rewrite the thing like this:

try:
    result = subprocess.check_output(cmd, stderr=subprocess.STDOUT, encoding='utf-8')
except subprocess.CalledProcessError as e:
    self.log.debug("Command: {} failed with the return code {} and the error {}".format(format_cmd_list(cmd), e.returncode, e.output))
    return False

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-06-13 14:04:36

Its MKDIR not PR_RENAME, and yes it is needed or else it silently fails.

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-06-13 14:06:02

Sure I can do that

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-06-13 20:40:18

rebased onto e6a061e93b44c2f46aea28b21aefce5f17d712a5

[389-ds-bot]

Comment from spichugi (@droideck) at 2018-06-13 23:36:28

My bad... I've looked at the wrong line in https://pagure.io/fork/mreynolds389/389-ds-base/blob/ticket49775/f/ldap/servers/slapd/back-ldbm/archive.c
You are right.

[389-ds-bot]

Comment from spichugi (@droideck) at 2018-06-14 00:08:05

I've tested the change and it works. Beside the small thing we discussed (debugging output for other subprocess.check_output changes - the same way as the first one), LGTM! Ack.

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-06-14 02:09:08

rebased onto 5226bf9

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-06-14 02:09:48

Pull-Request has been merged by mreynolds389

[389-ds-bot]

Patch
49776.patch