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

all: Bump minimum Go version to 1.19 #266

Merged
merged 6 commits into from
Mar 3, 2023
Merged

all: Bump minimum Go version to 1.19 #266

merged 6 commits into from
Mar 3, 2023

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Feb 28, 2023

Reference: #265

Bumped via:

go mod edit -go=1.19
go mod tidy
go fix ./...

@bflad bflad added dependencies Pull requests that update a dependency file go Pull requests that update Go code labels Feb 28, 2023
@bflad bflad added this to the v0.15.0 milestone Feb 28, 2023
@bflad bflad requested a review from a team as a code owner February 28, 2023 18:03
@bflad
Copy link
Contributor Author

bflad commented Feb 28, 2023

It looks like the Go update is awkwardly causing Go comment differences in the Protocol Buffers files between local and CI compilation, which unfortunately are not easily reconcilable (e.g. the first one is a protoc generated comment):

--- a/tfprotov5/internal/tfplugin5/tfplugin5.pb.go
+++ b/tfprotov5/internal/tfplugin5/tfplugin5.pb.go
@@ -1061,6 +1061,7 @@ type AttributePath_Step struct {
 	unknownFields protoimpl.UnknownFields
 
 	// Types that are assignable to Selector:
+	//
 	//	*AttributePath_Step_AttributeName
 	//	*AttributePath_Step_ElementKeyString
 	//	*AttributePath_Step_ElementKeyInt
@@ -2486,9 +2487,9 @@ type PlanResourceChange_Response struct {
 	// specific details of the legacy SDK type system, and are not a general
 	// mechanism to avoid proper type handling in providers.
 	//
-	//     ====              DO NOT USE THIS              ====
-	//     ==== THIS MUST BE LEFT UNSET IN ALL OTHER SDKS ====
-	//     ====              DO NOT USE THIS              ====
+	//	====              DO NOT USE THIS              ====
+	//	==== THIS MUST BE LEFT UNSET IN ALL OTHER SDKS ====
+	//	====              DO NOT USE THIS              ====
 	LegacyTypeSystem bool `protobuf:"varint,5,opt,name=legacy_type_system,json=legacyTypeSystem,proto3" json:"legacy_type_system,omitempty"`
 }

Running locally, it will remove the extra line of comment, for example. The other changes we could potentially update the definition files.

The upstream setup-protoc action has been unable to use newer protoc versions since the protoc versioning scheme was switched, although maybe that support will land soon with arduino/setup-protoc#58.

@bflad
Copy link
Contributor Author

bflad commented Mar 3, 2023

I was able to sort out my local generation issue. I had run brew install protoc-gen-go back on Go 1.18 and since brew builds from source, so it was using that older Go version comment formatting. brew reinstall protoc-gen-go did the trick.

This is ready for review and will handle #268 afterwards (Homebrew already has the new version).

Copy link
Contributor

@bendbennett bendbennett left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@bflad bflad merged commit 8598035 into main Mar 3, 2023
@bflad bflad deleted the bflad/go-upgrade branch March 3, 2023 18:02
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dependencies Pull requests that update a dependency file go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants