Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

gps: fix panic for gopkgin's implicit v0 #1243

Merged
merged 4 commits into from
Nov 20, 2017

Conversation

carolynvs
Copy link
Collaborator

@carolynvs carolynvs commented Oct 6, 2017

What does this do / why do we need it?

When v0 is requested, and there are no semver branches/tags, fallback to the master branch.

See http://labix.org/gopkg.in#VersionZero

What should your reviewer look out for in this PR?

Nope.

Do you need help or clarification on anything?

Nope.

Which issue(s) does this PR fix?

Fixes #933
Fixes #776

@carolynvs
Copy link
Collaborator Author

I opened #1244 for the AppVeyor failure, it may be a flakey test. I don't seem to be able to restart builds anymore on AppVeyor to test out that theory though.

Copy link
Collaborator

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

LGTM!

@darkowlzz darkowlzz closed this Oct 16, 2017
@darkowlzz darkowlzz reopened this Oct 16, 2017
@darkowlzz darkowlzz added this to the v0.3.3 milestone Oct 20, 2017
@darkowlzz
Copy link
Collaborator

darkowlzz commented Oct 20, 2017

Let's add this as a bug fix in the changelog.

if tv.sv.Major() == s.major && !s.unstable {
vlist[k] = v
k++
}
case branchVersion:
if tv.name == "master" {
Copy link
Member

@sdboyer sdboyer Oct 24, 2017

Choose a reason for hiding this comment

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

this binds us to the literal branch name master, which isn't guaranteed to be right. despite the gopkg.in docs on their site, it's also not actually what they do:

https://github.com/niemeyer/gopkg/blob/c70d35213a11190a3f9d921a421d4ec04620e60f/main.go#L410-L418

in the event that nothing matches, they basically just pass the data from upstream straight through.

we may be able to do something less complicated than what we have in gitSource.listVersions(), but we have to rely on the HEAD value in the output list.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for catching that. They saying master in the docs, and in the code comments, and in some hard-coded strings so I thought that's what they were using.

@carolynvs carolynvs force-pushed the gopkgin-v0-panic branch 3 times, most recently from 201cb87 to 8dc5eb9 Compare October 27, 2017 19:56
@carolynvs
Copy link
Collaborator Author

I have updated the logic to use the default branch returned from ListVersions, and tweaked my test repo to not have have a master branch.

@carolynvs
Copy link
Collaborator Author

I've rebased for great goodness.

When v0 is requested, and there are no semver branches/tags,
fallback to the default branch.

See http://labix.org/gopkg.in#VersionZero
@googlebot
Copy link
Collaborator

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@sdboyer sdboyer merged commit 13df556 into golang:master Nov 20, 2017
@carolynvs carolynvs deleted the gopkgin-v0-panic branch November 26, 2017 17:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
4 participants