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
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
5 changes: 2 additions & 3 deletions Documentation/dev-dependencies.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,10 @@ To add a new dependency to dex or update an existing one:

Tests will fail if transitive dependencies aren't included.

Once `glide.yaml` describes the desired state use glide and glide-vc to update `glide.lock` and `vendor`.
Once `glide.yaml` describes the desired state use `make` to update `glide.lock` and `vendor`. This calls both `glide` and `glide-vc` with the set of flags that dex requires.

```
glide up -v
glide-vc
make revendor
```

When composing commits make sure that updates to `vendor` are in a separate commit from the main changes. GitHub's UI makes commits with a large number of changes unreviewable.
Expand Down
5 changes: 5 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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 :/

@glide up -v
@glide-vc --use-lock-file --no-tests --only-code

test:
@go test -v -i $(shell go list ./... | grep -v '/vendor/')
@go test -v $(shell go list ./... | grep -v '/vendor/')
Expand Down
15 changes: 13 additions & 2 deletions glide_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"os"
"path"
"path/filepath"
"strings"
"testing"

"gopkg.in/yaml.v2"
Expand Down Expand Up @@ -96,13 +97,23 @@ func TestGlideYAMLPinsAllDependencies(t *testing.T) {
}
}

func TestRemoveVersionControl(t *testing.T) {
func TestGlideVCUseLockFile(t *testing.T) {
_, err := os.Stat("vendor/github.com/golang/protobuf/protoc-gen-go")
if err != nil {
t.Fatalf("vendor did not use glide-vc --use-lock-file. Revendor packages using 'make revendor' to use the correct glide and glide-vc flags")
}
}

func TestGlideFlagsAndGlideVC(t *testing.T) {
err := filepath.Walk("vendor", func(path string, info os.FileInfo, err error) error {
if err != nil {
t.Fatalf("walk: stat path %s failed: %v", path, err)
}
if info.IsDir() && filepath.Base(path) == ".git" {
t.Fatalf(".git directory detected in vendor: %s. Revendor packages and remove version control data with 'glide update -s -v -u'", path)
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.

}
return nil
})
Expand Down