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

Regenerate with protoc-gen-go 1.3.5 #572

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jsafrane
Copy link
Contributor

Regenerated csi.pb.go with older protoc-gen-go to restore grpc message structures.

go.mod has github.com/golang/protobuf 1.5.3, which has a protoc-gen-go that generates code that introduces breaking changes, see #570.

Instead of 1.5.3, download github.com/golang/protobuf v1.3.2 separately, build protoc-gen-go from it and use it to generate csi.pb.go. The code is copied from the previous release of CSI spec, only with minor changes.

A future update to 1.5.3 must be done carefully and announce a breaking change.

Which issue(s) this PR fixes:

Fixes #570

Does this PR introduce an API-breaking change?:

Restored generated protobuf go message structures compatible with CSI spec v1.9.0.

jsafrane added a commit to jsafrane/external-provisioner that referenced this pull request Aug 26, 2024
Use CSI spec from
container-storage-interface/spec#572 to test the
provisioner with a fixed CSI structures.
Copy link
Contributor

@huww98 huww98 left a comment

Choose a reason for hiding this comment

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

Should run go mod tidy to move github.com/golang/protobuf to direct dep.

Sorry for the inconvenience of introducing breaking changes. Could you suggest an alternative way to release the upgrade in the future? Maybe split the golang generated code to another repo (or place the go.mod in lib/go subdirectory), and only release v0 for the new module. If someone want old version of protoc-gen-go, he can always generate his own structs.

lib/go/Makefile Outdated Show resolved Hide resolved
@@ -46,8 +46,6 @@ PROTOC_URL := https://github.com/protocolbuffers/protobuf/releases/download/v$(P
PROTOC_TMP_DIR := .protoc
PROTOC := $(PROTOC_TMP_DIR)/bin/protoc

$(GOBIN)/protoc-gen-go: ../../go.mod
go install google.golang.org/protobuf/cmd/protoc-gen-go
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just change this line to

go install github.com/golang/protobuf/protoc-gen-go@v1.3.5

So we don't need any other changes to the Makefile

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 did not work for me, it wanted to downgrade other packages in go.mod

Copy link
Contributor

Choose a reason for hiding this comment

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

That's strange. I tried this and it works for me. github.com/golang/protobuf stays at v1.5.3 in go.mod

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your're right, I was installing google.golang.org/protobuf/cmd/protoc-gen-go@v1.3.5 and not github.com/golang/protobuf/protoc-gen-go@v1.3.5.

I updated the PR, now it's a super simple PR.

@huww98
Copy link
Contributor

huww98 commented Aug 27, 2024

Also note this PR does not revert the breaking change of requiring embedding UnimplementedIdentityServer.

go.mod has github.com/golang/protobuf 1.5.3, which has a protoc-gen-go that is
not compatible with csi.pb.go in this repo.

Download github.com/golang/protobuf v1.3.5 separately, build
protoc-gen-go from it and use it to generate csi.pb.go.

The code is copied from the previous release of CSI spec, only with minor
changes.

A future update to 1.5.3 must be done carefuly and announce a breaking change.
Copy link

@OKEAMAH OKEAMAH left a comment

Choose a reason for hiding this comment

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

Z

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1.10 generated files breaking changes
3 participants