rethinkdb-dump is broken

[epequeno]

Describe the bug
v2.4.3 tries to connect to localhost and ignores commandline arguments pointing to another host
v2.4.0 - v2.4.2.post1 error at check_tls_option()
v2.3.0.post6 last working version

To Reproduce
Steps to reproduce the behavior:

1. run rethinkdb-dump -c rethinkdb:28015 -f dump.tar.gz --tls-cert ./tls/rethinkdb-cert.pem --password-file pw

Expected behavior
A dump archive is successfully created without error

Errors
v2.4.3

$ rethinkdb-dump -c rethinkdb:28015 -f dump_2019-08-24.tar.gz --tls-cert ./tls/rethinkdb-cert.pem --password-file pw
Usage: rethinkdb dump [-c HOST:PORT] [-p] [--password-file FILENAME] [--tls-cert FILENAME] [-f FILE] [--clients NUM] [-e (DB | DB.TABLE)]...

rethinkdb-dump: error: Unable to connect to server: Could not connect to localhost:28015. Error: [Errno 99] Cannot assign requested address

v2.4.0 - v2.4.2.post1

$ rethinkdb-dump -c rethinkdb -f dump_2019-08-24.tar.gz --tls-cert ./tls/rethinkdb-cert.pem --password-file pw
Traceback (most recent call last):
  File "/usr/local/bin/rethinkdb-dump", line 10, in <module>
    sys.exit(main())
  File "/usr/local/lib/python3.7/site-packages/rethinkdb/_dump.py", line 143, in main
    options = parse_options(argv or sys.argv[2:], prog=prog)
  File "/usr/local/lib/python3.7/site-packages/rethinkdb/_dump.py", line 94, in parse_options
    options, args = parser.parse_args(argv)
  File "/usr/local/lib/python3.7/site-packages/rethinkdb/utils_common.py", line 356, in parse_args
    options, args = super(CommonOptionsParser, self).parse_args(*args, **kwargs)
  File "/usr/local/lib/python3.7/optparse.py", line 1387, in parse_args
    stop = self._process_args(largs, rargs, values)
  File "/usr/local/lib/python3.7/optparse.py", line 1427, in _process_args
    self._process_long_opt(rargs, values)
  File "/usr/local/lib/python3.7/optparse.py", line 1501, in _process_long_opt
    option.process(opt, value, values, self)
  File "/usr/local/lib/python3.7/optparse.py", line 779, in process
    value = self.convert_value(opt, value)
  File "/usr/local/lib/python3.7/optparse.py", line 771, in convert_value
    return self.check_value(opt, value)
  File "/usr/local/lib/python3.7/optparse.py", line 766, in check_value
    return checker(self, opt, value)
TypeError: check_tls_option() takes 2 positional arguments but 3 were given

System info
docker container built using FROM python:3

• OS:

$ cat /etc/os-release 
PRETTY_NAME="Debian GNU/Linux 10 (buster)"
NAME="Debian GNU/Linux"
VERSION_ID="10"
VERSION="10 (buster)"
VERSION_CODENAME=buster
ID=debian
HOME_URL="https://www.debian.org/"
SUPPORT_URL="https://www.debian.org/support"
BUG_REPORT_URL="https://bugs.debian.org/"

• Python version:

$ python -V
Python 3.7.4

[34code]

same issue

[gabor-boros]

Dear @epequeno,
Thank you for reporting the issue. I seen this behavior before and we already fixed it once, so it’s strange. Unfortunately I cannot investigate it today but tomorrow this will be my first thing to do. Also I’ll need to write test for the commands we have. Unfortunately we lack lot of tests

[gabor-boros]

Ok, I tried to find my previous findings. So, the first argument is ignored (or it should be “dump”). This is why it tries to connect to localhost, because the parameter -c is ignored.

The previous issue is #104. Unfortunately I did not realized that the discussion continued after I closed the issue, so I’ll need to touch this topic again.

A long term solution would be to rewrite these command because they are (sometimes) over complicated, extremely hard to maintain and not tested at all

[34code]

My temporary solution was to to pip3 install rethinkdb==2.3.0.post6 and do the export. I was then successfully able to import the data into rethinkdb too.

[alexa-infra]

I think these issues (this and #104) have been fixed in PR #132, please try master branch instead of pypi version

[gabor-boros]

It turned out that what @alexa-infra fixed is totally OK, but we have the other issue: how we provide the scripts installed by setup.py. With 2.3.0.post6 we installed only one script: rethinkdb, but now we are installing several commands: rethinkdb-dump, rethinkdb-import and so on. My harsh guess would be that this change was done because naming conflicts.

We have two options here:

• Keep the naming with dashes
• Fallback to the original naming

The root cause of the issue is that the optparser wants to parse the arguments like this: options = parse_options(argv or sys.argv[2:], prog=prog), because in the past we called the script with "method" as a first argument like: rethinkdb dump .... Due to we are not calling a single entrypoint the mentioned argument parsing will ignore the first parameter. So we will parse -c abcd:123 as abcd:123 (ignoring -c which would indicate connect) so it just skips this connection param.

I would suggest to keep the dashed script names, indicate in the readme what kind of scripts we have, and parse the first argument as well.

[alexa-infra]

@gabor-boros It's possible to provide scripts like

import sys
from rethinkdb._dump import main
if __name__ == '__main__':
  sys.exit(main(sys.argv[1:]))

instead of default shim files generated by setuptools for console_scripts 'rethinkdb-dump=rethinkdb._dump:main' etc. If it is called by rethinkdb-dump -c hostname then it would pass sys.argv[1:], and if it's called by rethinkdb dump -c hostname, then rethinkdb.__main__ correctly passes sys.argv[2:]

[gabor-boros]

@alexa-infra Yes, you are right. We should do this for the scripts. Also, I found that the other scripts parse the options in argv[1:] way. We will need to adjust all the scripts.

[gabor-boros]

@alexa-infra could you please check #139 to see if you have any other ideas?

Basically what you mentioned in this thread earlier (sys.argv[1:] vs sys.argv[2:]) is already implemented in the __main__.py as far as I can see. (https://github.com/rethinkdb/rethinkdb-python/blob/master/rethinkdb/__main__.py#L76 and https://github.com/rethinkdb/rethinkdb-python/blob/master/rethinkdb/_dump.py#L225)

[gabor-boros]

@alexa-infra and @epequeno, the fix is released in 2.4.3.post1. Could you please confirm that the issue is fixed? Thank you in advance.

[alexa-infra]

@gabor-boros confirmed

[gabor-boros]

Due to the resolution is confirmed, I'm closing this issue