Skip to content

Add leeway script to update protobuf generated code #3669

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

Merged
merged 3 commits into from
Apr 7, 2021
Merged

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Mar 30, 2021

Adds a new leeways script to run the generate.sh script: leeway run components:generate-code-from-protobuf

fixes #3630

Issue:

2021/03/30 20:54:39 WARNING: proto: file "workspace.proto" is already registered
	previously from: "github.com/gitpod-io/gitpod/content-service/api"
	currently from:  "github.com/gitpod-io/gitpod/ws-daemon/api"
A future release will panic on registration conflicts. See:
https://developers.google.com/protocol-buffers/docs/reference/go/faq#namespace-conflict

Also, implement change grpc/grpc-go#3669

@aledbf aledbf force-pushed the aledbf/protobuf branch 3 times, most recently from aad438e to 10d44c6 Compare March 30, 2021 20:47
@aledbf aledbf requested a review from csweichel March 30, 2021 21:17
@aledbf
Copy link
Member Author

aledbf commented Mar 31, 2021

/werft run

👍 started the job as gitpod-build-aledbf-protobuf.5

@aledbf aledbf added the type: improvement Improves an existing feature or existing code label Mar 31, 2021
@csweichel
Copy link
Contributor

Also removes pending manual code from .pb.go files into an explicit file not_autogenerated.XXXXXX.pb.go.

I don't understand this change. All of the .pb.go files should contain only auto-generated code. If those files don't line up with the code generation in this PR (or even on main) something fundamentally changed in the code generation and we should investigate if those files are still required.

@aledbf aledbf force-pushed the aledbf/protobuf branch 7 times, most recently from 7960352 to 4f9afc9 Compare April 6, 2021 12:30
@aledbf
Copy link
Member Author

aledbf commented Apr 6, 2021

I don't understand this change. All of the .pb.go files should contain only auto-generated code. If those files don't line up with the code generation in this PR (or even on main) something fundamentally changed in the code generation and we should investigate if those files are still required.

Fixed. The generate.sh was missing the grpc plugin (that generates the grpc client)

@aledbf aledbf mentioned this pull request Apr 6, 2021
24 tasks
Copy link
Contributor

@csweichel csweichel left a comment

Choose a reason for hiding this comment

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

Thank you for cleaning this up.

@@ -22,6 +22,8 @@ import (
type BlobService struct {
cfg storage.Config
s storage.PresignedAccess

api.UnimplementedBlobServiceServer
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand this change is necessary due to some deliberately non-backwards compatible, idiosyncratic design decisions (grpc/grpc-go#3669 (comment)).

I'm am however not convinced the unimplemented default implementation is preferable to things breaking at compile time. If someone modifies the interface, they should be made aware of the consequences of that action as early as possible.

We can't disable this behaviour though, so we'll have to live with it.

aledbf added 3 commits April 7, 2021 05:43
Add leeway script to update protobuf generated code
Update generated code
Embed the Unimplemented implementation
@aledbf aledbf force-pushed the aledbf/protobuf branch from 4f9afc9 to 9cb51aa Compare April 7, 2021 09:46
@aledbf aledbf merged commit dfda855 into main Apr 7, 2021
@aledbf aledbf deleted the aledbf/protobuf branch April 7, 2021 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: improvement Improves an existing feature or existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

proto: file "workspace.proto" is already registered
2 participants