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

Verify version during build #631

Merged
merged 6 commits into from
Oct 11, 2016
Merged

Verify version during build #631

merged 6 commits into from
Oct 11, 2016

Conversation

HelloGrayson
Copy link
Contributor

@HelloGrayson HelloGrayson commented Oct 5, 2016

Let's verify that the glide.Version matches the changelog - this will force us to update this value before releasing and prevent #629 from happening again.

To use this correctly, you must:

  1. The latest version listen in the CHANGELOG must match glide.Version.
  2. -dev can optionally be added to glide.Version.
  3. The latest unreleased version must be listed in the CHANGELOG.

We've used this strategy successfully in:

cc @mattfarina

@HelloGrayson
Copy link
Contributor Author

This first build should fail, verifying the feature. Once it does, I'll update the value of glide.Version to make it pass.

package main

// Version is the current version of Glide
const Version = "0.13.0-dev"

Choose a reason for hiding this comment

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

can unexport this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be useful for exterior libs to be able to get at this, in the case that they use glide as a lib.

Choose a reason for hiding this comment

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

I'd prefer to keep the visibility the same as it was previously as part of this change.

However since this is a main package, other's can't import it anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Point taken - I updated to maintain the old api!

@sdboyer
Copy link
Member

sdboyer commented Oct 6, 2016

i haven't thought through all the implications of this, but in general it seems like a great idea

@HelloGrayson
Copy link
Contributor Author

@sdboyer the implications is that the build will fail if go.version do not match the latest version in the changelog. In practice, this means catching issues like #629 from happening.

@sdboyer
Copy link
Member

sdboyer commented Oct 6, 2016

@breerly yes, that i understand - that's the intended effect. when i use "implication," i'm referring to potential second-order, unintended effects.

this isn't a knock against the PR; it's just a caveat about the depth of my review.

@@ -34,7 +34,7 @@ import (
"os"
)

var version = "0.13.0-dev"
const version = "0.13.0-dev"
Copy link
Member

Choose a reason for hiding this comment

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

By changing from a var to const the ldflags for -X main.version=${VERSION} no longer works in the make build. This is why it was a var in the first place.

I think this needs to be switched back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@mattfarina mattfarina added this to the 0.13 milestone Oct 10, 2016
@mattfarina
Copy link
Member

Once make build can pass the version into the code again this looks ready.

I like having this check.

@HelloGrayson
Copy link
Contributor Author

Got it, I'll switch back to var

@mattfarina mattfarina merged commit 21ff6d3 into Masterminds:master Oct 11, 2016
@mattfarina
Copy link
Member

@breerly thanks for the contribution.

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