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.

Sign up for GitHub

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

Jump to bottom

build: update to go1.19 #14132

Merged
merged 2 commits into from
Aug 16, 2022
Merged

build: update to go1.19 #14132

merged 2 commits into from
Aug 16, 2022

Conversation

shoenig
Copy link
Member

@shoenig shoenig commented Aug 16, 2022 •
edited
Loading

aebd61f - update to go1.19 (ok to backport)

1384e16f61be5a01ce762ff7a3618a33e4f3d96e - run gofmt (do not backport- do this manually for each one)

The gofmt command is just go fmt ./... and remove the changes to bindata files (protos ok)

Closes #14131

@shoenig shoenig changed the title [no ci] build: update to go1.19 build: update to go1.19 Aug 16, 2022
@shoenig shoenig marked this pull request as ready for review August 16, 2022 14:31
@shoenig shoenig requested review from tgross, lgfa29 and DerekStrickland and removed request for tgross August 16, 2022 14:31
@shoenig
Copy link
Member Author

shoenig commented Aug 16, 2022

I'll open the followup PRs for backports

shoenig
shoenig commented Aug 16, 2022
View reviewed changes
e2e/consultemplate/consultemplate.go
@@ -89,7 +89,7 @@ func (tc *ConsulTemplateTest) AfterEach(f *framework.F) {
// - missing keys block allocations from starting
// - key updates trigger re-render
// - service updates trigger re-render
// - 'noop' vs ''restart' configuration
// - 'noop' vs ”restart' configuration
Copy link
Member Author

Choose a reason for hiding this comment

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

golang/go#54312 🤦‍♂️

lgfa29
lgfa29 approved these changes Aug 16, 2022
View reviewed changes
Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

I don't know if the changes I pointed out can be fixed without gofmt complaining 😅

client/state/upgrade.go Outdated
Comment on lines 85 to 92
// * allocations
// * 15d83e8a-74a2-b4da-3f17-ed5c12895ea8
// * echo
// - simple-all (342 bytes)
// - alloc (2827 bytes)
// - alloc-dir (166 bytes)
// - immutable (15 bytes)
// - mutable (1294 bytes)
//
// - allocations
// - 15d83e8a-74a2-b4da-3f17-ed5c12895ea8
// - echo
// - simple-all (342 bytes)
// - alloc (2827 bytes)
// - alloc-dir (166 bytes)
// - immutable (15 bytes)
// - mutable (1294 bytes)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the indentation here is relevant? Is that a bug in gofmt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, I think we can just eliminate the dashes so gofmt doesn't assume this is a list

command/agent/consul/check_watcher_test.go Outdated
//func (c *fakeCheckRestarter) Restart(source, reason string, failure bool) {
// func (c *fakeCheckRestarter) Restart(source, reason string, failure bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it can be deleted altogether? 😅

command/agent/consul/namespaces_client.go Outdated
Comment on lines 71 to 72
//
// enabled. (Coming in followup PR).
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's how it should look like?

Suggested change
//
// enabled. (Coming in followup PR).
// enabled. (Coming in followup PR).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll just make the whole thing one line

helper/pluginutils/hclutils/testing.go Outdated
@@ -40,7 +40,7 @@ func (b *HCLParser) WithVars(vars map[string]cty.Value) *HCLParser {
// out parameter should be a golang reference to a driver specific TaskConfig reference.
// The function terminates and reports errors if any is found during conversion.
//
// Sample invocation would be
// # Sample invocation would be
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this would fix the recommendation? 🤷

Suggested change
// # Sample invocation would be
// Sample invocation would be:

nomad/state/paginator/tokenizer.go Outdated
Comment on lines 38 to 57
// StructsTokenizerOptions {
// WithID: true,
// }
// StructsTokenizerOptions {
// WithID: true,
// }
//
// - Structs that are only unique within their namespace:
//
// StructsTokenizerOptions {
// WithID: true,
// WithNamespace: true,
// }
// StructsTokenizerOptions {
// WithID: true,
// WithNamespace: true,
// }
//
// - Structs that can be sorted by their create index should also set
// `WithCreateIndex` to `true` along with the other options:
//
// StructsTokenizerOptions {
// WithID: true,
// WithNamespace: true,
// WithCreateIndex: true,
// }
// StructsTokenizerOptions {
// WithID: true,
// WithNamespace: true,
// WithCreateIndex: true,
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

figured it out - need to indent by 4 spaces to be in a code block

.go-version
@@ -1 +1 @@
1.18.5
1.19
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be good to also set a patch version?

Copy link
Member Author

Choose a reason for hiding this comment

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

the .0 release chops off the .0 (we do the same thing!)

@shoenig
build: run gofmt on all go source files
0c62f44 
Go 1.19 will forecefully format all your doc strings. To get this
out of the way, here is one big commit with all the changes gofmt
wants to make.
@shoenig shoenig added backport/1.2.x backport to 1.1.x release line backport/1.3.x backport to 1.3.x release line labels Aug 16, 2022
@shoenig shoenig merged commit 4e3c3d4 into main Aug 16, 2022
@shoenig shoenig deleted the build-update-go1.19 branch August 16, 2022 16:20
This was referenced Aug 16, 2022
Merged
Merged
@schmichael schmichael mentioned this pull request Aug 16, 2022
Merged
@jrasell jrasell mentioned this pull request Oct 31, 2022
Closed
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 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 Dec 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lgfa29 lgfa29 lgfa29 approved these changes

@DerekStrickland DerekStrickland Awaiting requested review from DerekStrickland

@tgross tgross Awaiting requested review from tgross

Assignees
No one assigned
Labels
backport/1.2.x backport to 1.1.x release line backport/1.3.x backport to 1.3.x release line
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update to go1.19 and format doc strings
2 participants
@shoenig @lgfa29