Skip to content

Conversation

@dnephin
Copy link

@dnephin dnephin commented Sep 18, 2015

Instead of having to update the docker versions we test against, query the github api and test against the most recent releases for the last two major.minor releases.

Copy link

Choose a reason for hiding this comment

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

Could you expand this out to not be on one line please? I'm not sure I've properly understood what's the intent and why you need to do this calculation. rc will already be an empty string if it didn't exist after the parse function right? Why do you want a (None, self.rc) returned. What piece of context have I missed.

Copy link
Author

Choose a reason for hiding this comment

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

This could definitely use a docstring to explain it.

The issue is that "default ordering" for this object would put rc releases after the canonical release. This (None, self.rc) forces any release that was an rc release to come before the canonical (non-rc) release because None is sorted to the front of the sequence before any other character.

Copy link

Choose a reason for hiding this comment

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

Ah right, I wasn't expecting that None would get sorted in front of any characters.

A docstring would also be helpful, I would still expand it out. It's not bash, you can have more than 1 line of code 👯 I feel having an if branch would help readability here.

Copy link

Choose a reason for hiding this comment

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

Yeah this is a bit of a puzzle to me, especially as I don't know the operator precedence between + and if off the top of my head.

Furthermore, I'd feel better if we didn't require readers of the code to know that fact about sorting. Instead of None and '', how about 0 and 1?

if self.rc:
    return (self.major, self.minor, self.patch, 0, self.rc)
else:
    return (self.major, self.minor, self.patch, 1)

I know it's verbose, but at least it's really clear what the return value will look like in both cases, so I don't have to construct a tuple in my head and consult the class definition when reading.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I've pushed a new version of this that I think makes it a lot more readable. It removes the need to know about operator precedence and it's explicit about the parts being returned on the return line.

@dnephin dnephin force-pushed the test_against_latest_version_tags branch from d410943 to e40b8b3 Compare October 5, 2015 20:08
Signed-off-by: Daniel Nephin <dnephin@docker.com>
@dnephin dnephin force-pushed the test_against_latest_version_tags branch from e40b8b3 to 6edb6fa Compare October 6, 2015 14:51
@dnephin
Copy link
Author

dnephin commented Oct 6, 2015

Added more docstrings with example responses and reference to the github api docs

@aanand
Copy link

aanand commented Oct 6, 2015

LGTM

aanand added a commit that referenced this pull request Oct 6, 2015
Test against a list of versions generated from docker/docker tags
@aanand aanand merged commit 02a20c0 into docker:master Oct 6, 2015
@dnephin dnephin deleted the test_against_latest_version_tags branch October 6, 2015 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants