Conversation
austinvalle
changed the title
| cmd := &exec.Cmd{ | ||
| Path: cmdLine[0], | ||
| Args: cmdLine[1:], | ||
| Args: cmdLine, |
There was a problem hiding this comment.
This is the bug fix, Args must always include the actual executable name/path as the first index. So the first index here (an actual argument!) was always being ignored 😆
This bug actually exists in TF core as well, but it seems the same --plugin path is duplicated, meaning that the first duplicate --plugin argument is actually ignored 😆. Core doesn't use protoc-gen-go-grpc (despite the variable naming), so it just duplicates the same path for protoc-gen-go.
In terraform-plugin-go we actually use both protoc-gen-go and protoc-gen-go-grpc, so after I fixed the variable/duplicate above, the first argument (protoc-gen-go) was being ignored, resulting in the "not found" error above.
There was a problem hiding this comment.
I think a contributor to the problem in core is some refactoring that arose after linting 🙈
I've proposed equivalent fixes on the Core side.
| log.Fatal(err) | ||
| } | ||
| protocGenGoGrpcExec, err := filepath.Abs(protocGenGoExec) | ||
| protocGenGoGrpcExec, err = filepath.Abs(protocGenGoGrpcExec) |
There was a problem hiding this comment.
This is the other part of the bug fix mentioned here
| // go.mod, as with all other Go dependencies. If you want to switch to a newer | ||
| // version of either tool then you can upgrade their modules in the usual way. | ||
| const protocGenGoPackage = "github.com/golang/protobuf/protoc-gen-go" | ||
| const protocGenGoPackage = "google.golang.org/protobuf/cmd/protoc-gen-go" |
There was a problem hiding this comment.
This isn't directly related to the bug fix, but the github Go module is the deprecated version which the main Go module doesn't use: https://pkg.go.dev/github.com/golang/protobuf/protoc-gen-go
SarahFrench
left a comment
SarahFrench
left a comment
There was a problem hiding this comment.
Looks good to me, and thanks again for finding the issues with the original script!
austinvalle
force-pushed
the
av/update-diff-ci
branch
from
f4bfdf2 to
3809bb8
Compare
3 participants
Related Issue
Ref: #569
Ref: #566
Description
This PR makes a couple changes, but the general theme is just fixing up our protobuf automation, changes made:
git diffcommand to detect only protocol changes that have not been regenerated, since there isn't really a need to check the entire repo for differences (like thego.sumfor example)go.workandgo.work.sumfiles, which aren't supported by dependabot and will drift over time unless we manually update them: Go: Support workspaces dependabot/dependabot-core#6012To prove the CI is working as expected, I modified the protocol without regenerating in this commit:
Rollback Plan
Changes to Security Controls
No