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

Fix make tools #6372

Closed
wants to merge 2 commits into from
Closed

Fix make tools #6372

wants to merge 2 commits into from

Conversation

ryboe
Copy link
Contributor

@ryboe ryboe commented May 13, 2020

NOTE: This includes all the changes from #6359 (the Go compiler update). #6359 should be reviewed and merged before this.

There was a slight regression from #6368, which removed vendor/. The misspell and golangci-lint tools are still being built out of vendor/, so make tools fails.

I updated make tools so that the compiled binaries are fetched from GitHub. Then I ran go mod tidy to remove unused packages from go.mod. It removed four packages. Removing dev dependencies from go.mod is a nice change for two reasons:

  1. The dependency solver will never hold back a package because of an incompatibility with the linter or misspell.
  2. Anybody importing packages from terraform-provider-google will not inherit the false dependency on golangci-lint or misspell.

@ghost ghost added the size/l label May 13, 2020
@ghost ghost requested a review from c2thorn May 13, 2020 20:38
go install github.com/golangci/golangci-lint/cmd/golangci-lint

curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(shell go env GOPATH)/bin v1.18.0
curl -L https://git.io/misspell | BINDIR=$(shell go env GOPATH)/bin bash
Copy link
Contributor Author

@ryboe ryboe May 13, 2020

Choose a reason for hiding this comment

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

Fetching the compiled binaries means we don't have to build them from source ourselves, which means we don't inherit their dependencies.

Both tools are installed to $GOPATH/bin.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @ryboe! Would you be able to provide me steps to reproduce make tools failing? I've cloned a clean repo and run it and it still succeeds for me. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake! make tools is working fine. I'm not sure why it was failing earlier for me. I'll close this PR. Thanks for catching this 🙂

import (
_ "github.com/client9/misspell/cmd/misspell"
_ "github.com/golangci/golangci-lint/cmd/golangci-lint"
)
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 underscore imports are how you vendor dev tools. Since we're not vendoring anymore, these should be deleted.

@c2thorn c2thorn requested review from megan07 and removed request for c2thorn May 13, 2020 20:54
ryboe added 2 commits May 18, 2020 12:19
Also simplify `make test` command.
Remove `GO111MODULE=on` and `GOFLAGS=-mod=vendor` env vars, which are no
longer necessary in Go 1.14.
There was a slight regression from #6368, which removed `vendor/`. The
`misspell` and `golangci-lint` tools were being built out of `vendor/`.
I updated `make tools` so that the compiled binaries are fetched from
GitHub. Then I ran `go mod tidy` to remove unused packages from `go.mod`.
It removed four packages. Removing dev dependencies from `go.mod` is a
nice change. The dependency solver will never artificially hold back a
package update because of an incompatibility with the linter or
`misspell`. Anybody importing packages from `terraform-provider-google`
will not inherit the false dependency on `golangci-lint` or `misspell`.
@ryboe
Copy link
Contributor Author

ryboe commented Jun 2, 2020

This PR was misguided. make tools works fine 👍

@ryboe ryboe closed this Jun 2, 2020
@ghost
Copy link

ghost commented Jul 3, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Jul 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants