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

*: add 'make revendor' and tests to catch incorrect glide usage #756

Merged
merged 2 commits into from
Dec 22, 2016

Conversation

ericchiang
Copy link
Contributor

@ericchiang ericchiang commented Dec 22, 2016

Introducing glide-vc caused us to unknowingly removed our Go protobuf
compiler (since it's a main). Add flags to glide-vc usage to remedy
this.

Since we now require several glide and glide-vc flags, add a Makfile
target and tests to catch when PRs don't use the correct flags.

cc @rithujohn191

@@ -35,6 +35,11 @@ bin/example-app: check-go-version
release-binary:
@go build -o _output/bin/dex -v -ldflags $(LD_FLAGS) $(REPO_PATH)/cmd/dex

.PHONY: revendor
revendor:
Copy link

Choose a reason for hiding this comment

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

Might be worth checking the version of glide used too, if you're trying to catch mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is kinda already addressed in our documentation.

Dex uses glide and glide-vc to manage its vendor directory. A recent version of these are preferred but dex doesn't require any bleeding edge features. Either install these tools using go get or take an opportunity to update to a more recent version.

If we ever need something that was added in a specific version of glide we can add a script similar to scripts/check-go-version, but I don't really want to write a bunch of sed commands right now :/

Eric Chiang added 2 commits December 22, 2016 11:52
Introducing glide-vc caused us to unknowingly removed our Go
protobuf compiler (since it's a main). Add flags to glide-vc usage
to remedy this.

Since we now require several glide and glide-vc flags, add a Makfile
target and tests to catch when PRs don't use the correct flags.
t.Fatalf(".git directory detected in vendor: %s. Revendor packages using 'make revendor' to use the correct glide and glide-vc flags", path)
}
if !info.IsDir() && strings.HasSuffix(path, "_test.go") {
t.Fatalf("'_test.go' file detected in vendor: %s. Revendor packages using 'make revendor' to use the correct glide and glide-vc flags", path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this so we avoid vendors from introducing their own tests into our code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. glide-vc has a mode that deletes all test files from the vendor directory. We don't run those tests anyway, so there's no good reason to hold onto them.

@rithujohn191
Copy link
Contributor

lgtm

@rithujohn191 rithujohn191 merged commit 3e2d857 into dexidp:master Dec 22, 2016
@ericchiang ericchiang deleted the revendor branch December 22, 2016 20:06
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.

None yet

3 participants