-
Notifications
You must be signed in to change notification settings - Fork 19
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
Regenerate generated and committed files #289
Conversation
to fix the following error: ``` Error: cws/dumpsv1/activity_dump_vtproto.pb.go:9:2: no required module provides package github.com/planetscale/vtprotobuf/protohelpers ```
/merge |
🚂 MergeQueue This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals. Use |
This merge request was unqueued If you need support, contact us on Slack #ci-interfaces! |
github.com/golang/protobuf v1.5.0 | ||
github.com/stretchr/testify v1.3.0 | ||
google.golang.org/protobuf v1.28.1 | ||
github.com/golang/protobuf v1.5.3 | ||
github.com/planetscale/vtprotobuf v0.6.0 | ||
github.com/stretchr/testify v1.8.4 | ||
google.golang.org/protobuf v1.31.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 could you provide some context around these module upgrades and the addition of planetscale/vtprotobuf? I understand gogo/protobuf is deprecated but to my knowledge we haven't decided on a pathforward with migrating off it
protobuf version updates are a risky change and needs to be coordinated with the dd-go backend services - there is a RFC listing the pain points around this and how an upgrade impacts other projects that import the agent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm I see we are already using vtprotobuf in the rake task but that it is unvendored
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intent wasn’t to update or migrate anything, but only to resynchronise what has been committed in the repository with what would be generated today.
Basically, if we checkout master
, don’t do any change at all, and run rake codegen
, it modifies generated and committed files.
This commit contains only the changes made by rake codegen
: 30215bd
These 100% generated changes are failing to compile with the following error:
Error: cws/dumpsv1/activity_dump_vtproto.pb.go:9:2: no required module provides package github.com/planetscale/vtprotobuf/protohelpers; to add it:
go get github.com/planetscale/vtprotobuf/protohelpers
The second commit of this PR contains no more than the outcome of this go get …/protohelpers
command: 27b43e5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the issue would be with the additional module constraints for users in dd-go importing the library
I checked and it looks like they are already updated to these versions so... I think we're fine? I'll resolve this thread
go.mod: github.com/gogo/protobuf v1.3.2
go.mod: github.com/golang/protobuf v1.5.3
go.mod: google.golang.org/protobuf v1.31.0
FYI: I've just merged #287 that pinned protoc-gen-go-vtproto to v0.5.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be possible to pin the go install
of vtprotobuf here to use what is in the mod file as we do with gogo/protobuf?
This is not needed anymore thanks to #287. |
What does this PR do?
Re-generate the generated and committed files with
rake codegen
.Motivation
Opening a PR like #287 or #288 to add one field in one payload generates a lot of diff caused by the update of the generator.
It’s better to isolate the diff caused by the generator update in a dedicated PR.
Additional Notes
Possible Drawbacks / Trade-offs
Describe how to test/QA your changes
Reviewer's Checklist
Reviewers: please see the review guidelines.