Add "checkgenerate" make target to CI #239
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jhump
commented
Apr 17, 2023
| @go install google.golang.org/protobuf/cmd/protoc-gen-go@a709e31e5d12 | ||
| @go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@v1.1.0 | ||
| @go install github.com/jhump/protoreflect/desc/sourceinfo/cmd/protoc-gen-gosrcinfo | ||
| @go install github.com/jhump/protoreflect/desc/sourceinfo/cmd/protoc-gen-gosrcinfo@v1.14.1 |
There was a problem hiding this comment.
This line wouldn't run without this explicit version. The v1.14.1 tag for this repo is already in go.mod, however there are some transitive deps of this package that are not otherwise used by this repo, so they had no entries in go.sum. This gets it working again.
dragonsinth
reviewed
Apr 18, 2023
download_protoc.sh
Outdated
| rm -rf ./.tmp/protoc | ||
| mkdir -p .tmp/protoc | ||
| curl -L "https://github.com/google/protobuf/releases/download/v${PROTOC_VERSION}/protoc-${PROTOC_VERSION}-${PROTOC_OS}-${PROTOC_ARCH}.zip" > .tmp/protoc/protoc.zip | ||
| cd ./.tmp/protoc && unzip protoc.zip && cd .. |
There was a problem hiding this comment.
Suggested change
| cd ./.tmp/protoc && unzip protoc.zip && cd .. | |
| cd ./.tmp/protoc && unzip protoc.zip && cd ../.. |
pushd/popd not generally available? just curious
dragonsinth
reviewed
Apr 18, 2023
download_protoc.sh
Outdated
|
|
||
| cd $(dirname $0) | ||
|
|
||
| PROTOC_VERSION="22.0" |
There was a problem hiding this comment.
Could we pass this value in from the Makefile for clarity?
What version were we using before? I noticed the outputs are changed.
jhump
commented
Apr 18, 2023
| @@ -89,7 +90,6 @@ errcheck: | |||
| test: | |||
| go test -race ./... | |||
|
|
|||
| .tmp/protoc/bin/protoc: | |||
| .tmp/protoc/bin/protoc: ./Makefile ./download_protoc.sh | |||
There was a problem hiding this comment.
I added the deps here, so that the compiler is auto-redownloaded if we change the version (which is in the Makefile) or the download script.
dragonsinth
approved these changes
Apr 18, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I was curious about the previously merged PR (#238), noticing that it only updated the JS code and not any Go code where the webform was embedded.
I now see that
go:embedis used, which is why no explicit Go code had to be changed. Nice!But while playing with it, I realized that
make generatewasn't being checked in CI, to make sure that all code was up to date from code generators. So this adds such a check to CI.