Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions script/test-versions
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,15 @@ docker run --rm \
--entrypoint="tox" \
"$TAG" -e pre-commit

ALL_DOCKER_VERSIONS="1.7.1 1.8.2"
DEFAULT_DOCKER_VERSION="1.8.2"
get_versions="docker run --rm
--entrypoint=/code/.tox/py27/bin/python
$TAG
/code/script/versions.py docker/docker"

if [ "$DOCKER_VERSIONS" == "" ]; then
DOCKER_VERSIONS="$DEFAULT_DOCKER_VERSION"
DOCKER_VERSIONS="$($get_versions default)"
elif [ "$DOCKER_VERSIONS" == "all" ]; then
DOCKER_VERSIONS="$ALL_DOCKER_VERSIONS"
DOCKER_VERSIONS="$($get_versions recent -n 2)"
fi


Expand Down
139 changes: 139 additions & 0 deletions script/versions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
#!/usr/bin/env python
"""
Query the github API for the git tags of a project, and return a list of
version tags for recent releases, or the default release.

The default release is the most recent non-RC version.

Recent is a list of unqiue major.minor versions, where each is the most
recent version in the series.

For example, if the list of versions is:

1.8.0-rc2
1.8.0-rc1
1.7.1
1.7.0
1.7.0-rc1
1.6.2
1.6.1

`default` would return `1.7.1` and
`recent -n 3` would return `1.8.0-rc2 1.7.1 1.6.2`
"""
from __future__ import print_function

import argparse
import itertools
import operator
from collections import namedtuple

import requests


GITHUB_API = 'https://api.github.com/repos'


class Version(namedtuple('_Version', 'major minor patch rc')):

@classmethod
def parse(cls, version):
version = version.lstrip('v')
version, _, rc = version.partition('-')
major, minor, patch = version.split('.', 3)
return cls(int(major), int(minor), int(patch), rc)

@property
def major_minor(self):
return self.major, self.minor

@property
def order(self):
"""Return a representation that allows this object to be sorted
correctly with the default comparator.
"""
# rc releases should appear before official releases
rc = (0, self.rc) if self.rc else (1, )
return (self.major, self.minor, self.patch) + rc

def __str__(self):
rc = '-{}'.format(self.rc) if self.rc else ''
return '.'.join(map(str, self[:3])) + rc


def group_versions(versions):
"""Group versions by `major.minor` releases.

Example:

>>> group_versions([
Version(1, 0, 0),
Version(2, 0, 0, 'rc1'),
Version(2, 0, 0),
Version(2, 1, 0),
])

[
[Version(1, 0, 0)],
[Version(2, 0, 0), Version(2, 0, 0, 'rc1')],
[Version(2, 1, 0)],
]
"""
return list(
list(releases)
Copy link

Choose a reason for hiding this comment

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

Are you using list here as a shorthand to ensure uniqueness? I would also expand this code out to be a little more verbose :)

Copy link
Author

Choose a reason for hiding this comment

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

lists aren't unique!

No, I'm using lists so that I can index into the sequence. itertools.groupby() returns a generator which can't be indexed.

I really dislike adding unnecessary code. This is pretty succinct and uses clear variables names. One of the great features of python is that you don't have to write overly verbose code to express your ideas. Every additional line of code is an opportunity for a bug to exist. Fewer lines of code is fewer bugs! I understand the need for clear variable names, and whitespace to separate ideas, but I think this function is better expressed in a functional way (as it is written) than expanding it out into multiple lines.

This function reads pretty well to me:

"List the releases for all versions grouped by the major_minor attribute".

Copy link

Choose a reason for hiding this comment

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

I made a stupid mistake, I was thinking of sets while reading this, thinking if we used sets then that would give you the unique versions. Anyway. Disregard that bit.

What I'm trying to express is that to me, expanding this out isn't unnecessary code. This is harder to read. It reads well to you because you wrote it. As someone reading it to grok the solution and what problem it is solving, I'm sharing with you that this approach is harder to read in that it takes longer to figure out what's happening here.

Balancing the line between very succinct code and over verbose code, we want somewhere in the middle. The rest of the codebase is written aiming for that somewhere in the middle. We don't have lots of functions that are inline like this, so I think it's fair to ask for it to be expanded out and made to be as readable as everything else we have.

Maybe worth pinging this @shin- and @aanand to see if it is just me who finds this harder to read. I appreciate my personal preferences and past experiences with code like this, means I'm not as much of a fan of denser functional coding style.

I'm not blocking the PR on this, I am expressing my opinion.

Copy link

Choose a reason for hiding this comment

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

I think an example of the return format in the docstring would go a long way.

>>> group_versions([
        Version.parse("1.0.0"),
        Version.parse("1.0.1"),
        Version.parse("1.1.0"),
        Version.parse("1.2.0"),
    ])
[
    [Version.parse("1.0.0"), Version.parse("1.0.1")],
    [Version.parse("1.1.0")],
    [Version.parse("1.2.0")],
]

In fact, docstring examples on most of these functions would be good, 'cause it's not known to the reader what the JSON returned by GitHub looks like.

for _, releases
in itertools.groupby(versions, operator.attrgetter('major_minor'))
)


def get_latest_versions(versions, num=1):
"""Return a list of the most recent versions for each major.minor version
group.
"""
versions = group_versions(versions)
return [versions[index][0] for index in range(num)]


def get_default(versions):
"""Return a :class:`Version` for the latest non-rc version."""
for version in versions:
if not version.rc:
return version


def get_github_releases(project):
"""Query the Github API for a list of version tags and return them in
sorted order.

See https://developer.github.com/v3/repos/#list-tags
"""
url = '{}/{}/tags'.format(GITHUB_API, project)
response = requests.get(url)
response.raise_for_status()
versions = [Version.parse(tag['name']) for tag in response.json()]
return sorted(versions, reverse=True, key=operator.attrgetter('order'))


def parse_args(argv):
parser = argparse.ArgumentParser(description=__doc__)
parser.add_argument('project', help="Github project name (ex: docker/docker)")
parser.add_argument('command', choices=['recent', 'default'])
parser.add_argument('-n', '--num', type=int, default=2,
help="Number of versions to return from `recent`")
return parser.parse_args(argv)


def main(argv=None):
args = parse_args(argv)
versions = get_github_releases(args.project)

if args.command == 'recent':
print(' '.join(map(str, get_latest_versions(versions, args.num))))
elif args.command == 'default':
print(get_default(versions))
else:
raise ValueError("Unknown command {}".format(args.command))


if __name__ == "__main__":
main()