Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

build: Use Go 1.19 for Terraform v1.3 #31655

Merged
merged 2 commits into from
Aug 22, 2022
Merged

build: Use Go 1.19 for Terraform v1.3 #31655

merged 2 commits into from
Aug 22, 2022

Conversation

apparentlymart
Copy link
Contributor

This will cause our official releases for the forthcoming v1.3 series to be built with Go 1.19 instead of Go 1.18 as before. This is in line with our usual policy of adopting the latest Go minor release available at the time when we freeze each Terraform minor release.


Go 1.19's "fmt" has some awareness of the new doc comment formatting conventions and adjusts the presentation of the source comments to make it clearer how godoc would interpret them. Therefore this commit includes various updates made by "go fmt" to acheve that.

In line with our usual convention that we make stylistic/grammar/spelling tweaks typically only when we're "in the area" changing something else anyway, I also took this opportunity to review most of the comments that this updated to see if there were any other opportunities to improve them.


I intend to include the following in our own changelog to pass on a relevant caveat from the Go 1.19 Release Notes:

  • When making requests to HTTPS servers, Terraform will now reject invalid handshakes that have duplicate extensions, as required by RFC 5246 section 7.4.1.4 and RFC 8446 section 4.2. This may cause new errors when interacting with existing buggy or misconfigured TLS servers, but should not affect correct servers.

    This only applies to requests made directly by Terraform CLI, such as provider installation and remote state storage. Terraform providers are separate programs which decide their own policy for handling of TLS handshakes.

There is also a change that might affect Terraform's visible behavior, but I think not in a way that's significant enough to call out explicitly:

  • The pure Go DNS resolver now uses a DNS extension to allow larger reply packets. The Go team acknowledges that this might interact poorly with old/buggy DNS servers, but doesn't seem to be super concerned about it.

@apparentlymart apparentlymart requested a review from a team August 17, 2022 19:19
@apparentlymart apparentlymart self-assigned this Aug 17, 2022
@apparentlymart
Copy link
Contributor Author

apparentlymart commented Aug 17, 2022

The Vercel build error here seems to be unrelated to this change and is complaining about an ENOMEM error when reading some files unrelated to this Go upgrade.

Update: I re-ran it and the failure went away.

@apparentlymart
Copy link
Contributor Author

apparentlymart commented Aug 17, 2022

Apparently something we're running in our code consistency checks is blocking us from using the deprecated functions in io/ioutil.

As discussed over in #31357, we were not planning to do a mass-update of those all at once in order to avoid suddently creating merge conflict hell for ourselves in case we need to backport to older Terraform versions. It seems that in order to proceed here we'll need to either change that decision or change the configuration of this checking tool to permit us to continue using these deprecated functions.

I would personally lean towards the latter, especially if we can combine it with some settings in our own editors to mark the deprecations as warnings so we can easily identify opportunities to improve when we're already in the area changing some nearby code anyway. But I'm not sure how practical either of those is, so will need to investigate further.

@jbardin
Copy link
Member

jbardin commented Aug 17, 2022

We can add ,-SA1019 to the staticcheck -checks options to specifically skip that analysis

@apparentlymart
Copy link
Contributor Author

I think retaining our existing policy ought to be the "default" answer here and so I've pushed a new commit to change the staticcheck options to agree with that policy.

We could still change the policy, but if we do that then it'll be in a separate PR which can both re-enable this rule in staticcheck and bulk-update all of the existing deprecation errors to allow us to move forward.

Go 1.19's "fmt" has some awareness of the new doc comment formatting
conventions and adjusts the presentation of the source comments to make
it clearer how godoc would interpret them. Therefore this commit includes
various updates made by "go fmt" to acheve that.

In line with our usual convention that we make stylistic/grammar/spelling
tweaks typically only when we're "in the area" changing something else
anyway, I also took this opportunity to review most of the comments that
this updated to see if there were any other opportunities to improve them.
Because we maintain multiple versions of Terraform across different
release branches, we aim to avoid creating needless differences between
the branches to maximize the chance of successful automatic backporting.

Part of that policy is that we don't make cross-cutting changes to respond
to deprecation of functions in upstream packages and instead we respond
to them gradually over time when we'd be changing the nearby code anyway,
or when new work requires using the replacement APIs.

In recognition of that, this turns of the staticcheck rule that would
otherwise force us to resolve all deprecations before moving forward with
any other change.
Copy link
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

I think we should address the protobuf issues (see inline note), but otherwise this looks good to me.

@apparentlymart apparentlymart merged commit f6cc907 into main Aug 22, 2022
@apparentlymart apparentlymart deleted the go1.19 branch August 22, 2022 17:59
@github-actions
Copy link
Contributor

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@github-actions
Copy link
Contributor

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 Sep 22, 2022
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants