Skip to content

Conversation

ijc
Copy link
Contributor

@ijc ijc commented Mar 28, 2019

The lstrip and rstrip functions take a set of characters to remove, not a
prefix/suffix. Thus rstrip('-x86_64') will remove any trailing characters in
the string '-x86_64' in any order (in effect it strips the suffix matching
the regex [-_x468]*). So with 18.09.4 it removes the 4 suffix resulting
in trying to int('') later on:

Traceback (most recent call last):
  File "/src/scripts/versions.py", line 80, in <module>
    main()
  File "/src/scripts/versions.py", line 73, in main
    versions, reverse=True, key=operator.attrgetter('order')
  File "/src/scripts/versions.py", line 52, in order
    return (int(self.major), int(self.minor), int(self.patch)) + stage
ValueError: invalid literal for int() with base 10: ''

Since we no longer need to check for the arch suffix (since it no longer
appears in the URLs we are traversing) we could just drop the rstrip and
invent a local prefix stripping helper to replace lstrip('docker-'). Instead
lets take advantage of the behaviour of re.findall which is that if the regex
contains a single () match that will be returned. This lets us match exactly
the sub-section of the regex we require.

While editing the regex, also ensure that the suffix is precisely .tgz and
not merely tgz by adding an explicit \., previously the literal . would
be swallowed by the .* instead.

Signed-off-by: Ian Campbell ijc@docker.com

The `lstrip` and `rstrip` functions take a set of characters to remove, not a
prefix/suffix. Thus `rstrip('-x86_64')` will remove any trailing characters in
the string `'-x86_64'` in any order (in effect it strips the suffix matching
the regex `[-_x468]*`). So with `18.09.4` it removes the `4` suffix resulting
in trying to `int('')` later on:

    Traceback (most recent call last):
      File "/src/scripts/versions.py", line 80, in <module>
        main()
      File "/src/scripts/versions.py", line 73, in main
        versions, reverse=True, key=operator.attrgetter('order')
      File "/src/scripts/versions.py", line 52, in order
        return (int(self.major), int(self.minor), int(self.patch)) + stage
    ValueError: invalid literal for int() with base 10: ''

Since we no longer need to check for the arch suffix (since it no longer
appears in the URLs we are traversing) we could just drop the `rstrip` and
invent a local prefix stripping helper to replace `lstrip('docker-')`. Instead
lets take advantage of the behaviour of `re.findall` which is that if the regex
contains a single `()` match that will be returned. This lets us match exactly
the sub-section of the regex we require.

While editing the regex, also ensure that the suffix is precisely `.tgz` and
not merely `tgz` by adding an explicit `\.`, previously the literal `.` would
be swallowed by the `.*` instead.

Signed-off-by: Ian Campbell <ijc@docker.com>
@ijc
Copy link
Contributor Author

ijc commented Mar 28, 2019

This issue is taking out all PR CI runs at the moment (since 18.09.4 was released).

Copy link
Contributor

@ulyssessouza ulyssessouza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ulyssessouza ulyssessouza merged commit f97c577 into docker:master Mar 28, 2019
@ijc ijc deleted the fix-docker-version-detection-script branch March 28, 2019 14:59
@shin-
Copy link
Contributor

shin- commented Mar 28, 2019

Holy carp. Sorry about that 😓 Thanks for getting a fix out @ijc !

@ijc
Copy link
Contributor Author

ijc commented Mar 28, 2019

No worries! The names of those functions are so misleading I'm surprised it doesn't happen more often!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants