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

Update golang version to 1.17 #1036

Merged
merged 4 commits into from
Jan 28, 2022
Merged

Conversation

ZPascal
Copy link

@ZPascal ZPascal commented Nov 26, 2021

Hi community, I've updated the deprecated old version of Golang to the current version.

@ZPascal
Copy link
Author

ZPascal commented Dec 14, 2021

Hi @ina-stoyanova @infraredgirl @marinalimeira @robmorgan, Is there a chance to get a review? Thx

@ina-stoyanova
Copy link
Contributor

Hey @ZPascal we'll prioritize this and send you a response! :) Thank you for the PR contribution :)

@denis256
Copy link
Member

Hi, with the go version update, I think should reformatted files using new rules - on this PR, after execution of gofmt from 1.17 changes on files appears, not sure if it should be done in this PR or separated.

Example:

$ cd test
$ ~/sdk/go1.17/bin/gofmt -w *.go
$ git status
        modified:   helm_basic_example_integration_test.go
        modified:   helm_basic_example_template_test.go
        modified:   kubernetes_basic_example_service_check_test.go
        modified:   kubernetes_basic_example_test.go
        modified:   kubernetes_hello_world_example_test.go
        modified:   kubernetes_rbac_example_test.go
$ git diff
...
@@ -1,3 +1,4 @@
+//go:build kubeall || kubernetes
 // +build kubeall kubernetes
+//go:build kubeall || helm
 // +build kubeall helm


https://go.dev/doc/go1.17#gofmt

@ZPascal
Copy link
Author

ZPascal commented Dec 16, 2021

@denis256 For my understanding, it's better to reformat the files in a separate PR. I can prepare a PR for that.

@yorinasub17
Copy link
Contributor

Thanks for the contribution! There are two issues that need to be resolved before we can merge this in:

Alternatively, you can set the go.mod version to 1.16 to avoid both of the above for now.

@ZPascal
Copy link
Author

ZPascal commented Dec 20, 2021

@yorinasub17 Thanks for the clarification and information. I will update my PR.

@ZPascal
Copy link
Author

ZPascal commented Dec 23, 2021

@yorinasub17 Is there a possibility to test the modified circle ci configuration?

@yorinasub17
Copy link
Contributor

Apologies for the delay! Just ran the build and looks like it failed on the precommit hook for go fmt, this time in the modules directory. Can you take a look at the list and format them? Thanks!

https://app.circleci.com/pipelines/github/gruntwork-io/terratest/842/workflows/f12868bb-d1ce-4d60-8823-304e75b307bb/jobs/6910

@yorinasub17
Copy link
Contributor

Thanks for the update! I kicked off the build, and it was able to get further, but there was still one more regression that needs to be addressed. All references to go 1.16.3 in https://github.com/gruntwork-io/terratest/tree/master/modules/logger/parser/fixtures need to be updated to point to 1.17.

Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! The rerun of the build passed, and this is almost ready to go. One final check we need to make is to make sure that this doesn't break our Azure testing.

@HadwaAbdelhalem when you get the chance, can you run this through our Azure GitHub Action and report the results? Thanks!

@HadwaAbdelhalem
Copy link
Contributor

@yorinasub17 Looks good to me I ran the CI, the failure is due to subscription limits.

@yorinasub17
Copy link
Contributor

@HadwaAbdelhalem Thanks for running!

Will merge this in now! @ZPascal thanks for the contribution and working through the long cycles!

@yorinasub17 yorinasub17 merged commit 858fcf2 into gruntwork-io:master Jan 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants