Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MySQL version needs to be decoded #50849

Open
5uper5hoot opened this issue Dec 13, 2018 · 2 comments

Comments

Projects
None yet
2 participants
@5uper5hoot
Copy link
Contributor

commented Dec 13, 2018

Using python3, the modules.mysql.version() function returns <class 'bytes'>, e.g. b'5.7.23-23-57-log'.

utils.versions.version_cmp() is used to compare this with '8.0.11' e.g.:

if salt.utils.versions.version_cmp(server_version, '8.0.11') >= 0:

...however that comparison always returns 1.

>>> import salt.utils.versions
>>> salt.utils.versions.version_cmp(b'5.7.23-23-57-log', '8.0.11')
1

The issue appears to come from the normalize lambda in version_cmp stringifying the b flag:

salt/salt/utils/versions.py

Lines 243 to 244 in 6ea059d

normalize = lambda x: six.text_type(x).split(':', 1)[-1] \
if ignore_epoch else six.text_type(x)

>>> import six
>>> ignore_epoch = False
>>> normalize = lambda x: six.text_type(x).split(':', 1)[-1] \
...                 if ignore_epoch else six.text_type(x)
>>> normalize(b'5.7.23-23-57-log')
"b'5.7.23-23-57-log'"
>>>
>>> from salt.utils.versions import LooseVersion
>>> LooseVersion("b'5.7.23-23-57-log'") > LooseVersion('8.0.11')
True
>>>

Decoding the bytes solves it...

>>> import salt.utils.data
>>> salt.utils.versions.version_cmp(salt.utils.data.decode(b'5.7.23-23-57-log'), '8.0.11')
-1

... and could likely be applied at the return statement of `modules.mysql.version(). I'm happy to send through a PR if OK.

Salt Version:
           Salt: 2018.3.0-n/a-6ea059d

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: Not Installed
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.10
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: 1.3.10
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 3.6.7 (default, Oct 22 2018, 11:32:17)
   python-gnupg: Not Installed
         PyYAML: 3.12
          PyZMQ: 16.0.2
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.2.5

System Versions:
           dist: Ubuntu 18.04 bionic
         locale: UTF-8
        machine: x86_64
        release: 4.15.0-42-generic
         system: Linux
        version: Ubuntu 18.04 bionic
@garethgreenaway

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

@5uper5hoot Thanks for the report and good find. If you could put together a PR with the above mentioned fix that would be greatly appreciated. Thanks!

@5uper5hoot

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

@garethgreenaway Regarding this, do you think it would be better for the conversion to happen in the versions.version_cmp() function as that is really where the issue is. Current PR only fixes it for the MySQL case, but not hard to imagine byte strings coming from other sources.

Or, perhaps to mention in the docstring that for version_cmp() that it doesn't handle bytes literals?

5uper5hoot added a commit to 5uper5hoot/salt that referenced this issue Jan 10, 2019

5uper5hoot added a commit to 5uper5hoot/salt that referenced this issue Jan 10, 2019

Ch3LL added a commit to Ch3LL/salt that referenced this issue Jan 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.