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 Go version and dependencies to ease development #312

Merged
merged 3 commits into from Apr 2, 2024

Conversation

kichristensen
Copy link
Contributor

What does this change

Because of old Go version and different dependencies not supporting newer Go versions, it takes some work to get things to compile. You will get weird errors when using newer versions of Go, none of them describes what the real issue is.
Updating the Go version to 1.21 allows it to be compiled with newer versions of the Go compiler, but requires different dependencies to be updated.
The Porter version and mixins referenced is also old, so it was updated to the newest versions.
External dependencies of, e.g., Kustomize, controller-gen and Ginkgo was also very old and no longer supported, so they were also updated.

Updates the following Go packages:

  • Ginkgo
  • Gomega
  • Porter
  • Porter Magefiles

Mixins:

  • Kubernetes
  • Helm3
  • Exec

External dependencies:

  • Operator SDK
  • Kustomize
  • controller-gen
  • Ginkgo

Checklist

  • Did you write tests?
  • Did you write documentation?
  • Did you make any API changes? Update the corresponding API documentation.

Update Go to version 1.21

Updates the following Go packages:
* Ginkgo
* Gomega
* Porter
* Porter Magefiles

Mixins:
* Kubernetes
* Helm3
* Exec

External dependencies:
* Operator SDK
* Kustomize
* controller-gen
* Ginkgo

Signed-off-by: Kim Christensen <kimworking@gmail.com>
Copy link

codecov bot commented Mar 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.90%. Comparing base (8038a94) to head (0c1a5e0).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #312   +/-   ##
=======================================
  Coverage   68.90%   68.90%           
=======================================
  Files          15       15           
  Lines        2248     2248           
=======================================
  Hits         1549     1549           
  Misses        548      548           
  Partials      151      151           
Flag Coverage Δ
unittests 68.90% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -1,6 +1,6 @@
module get.porter.sh/operator

go 1.20
go 1.21
Copy link
Member

Choose a reason for hiding this comment

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

Because you are updating this, it’ll help if you also update controller-runtime to v0.17. This dep is also at go version 1.21

@troy0820
Copy link
Member

This also means that we have to cut a new version of the operator. Older go versions will not be able to use this version as the new minimum required go version will be 1.21.

@bdegeeter
Copy link
Contributor

bdegeeter commented Mar 31, 2024 via email

Signed-off-by: Kim Christensen <kimworking@gmail.com>
@kichristensen
Copy link
Contributor Author

kichristensen commented Mar 31, 2024

Are specific features of the newer go version needed?

@bdegeeter From my point for view no, this PR is only meant to solve a few problems I had when trying to build the project. I had ~3 errors that took a long time to figure out, just because I had Go 1.22 installed. I can remember 2 of them off the top of my head, one was with staticcheck during linting and the other was with controller-gen.
The errors did not mention in anyway that it had something to do with the version of Go.

But @troy0820 might have better insights to you question

@bdegeeter
Copy link
Contributor

@bdegeeter From my point for view no, this PR is only meant to solve a few problems I had when trying to build the project. I had ~3 errors that took a long time to figure out, just because I had Go 1.22 installed. I can remember 2 of them off the top of my head, one was with staticcheck during linting and the other was with controller-gen.
The errors did not mention in anyway that it had something to do with the version of Go.

Thanks for clarifying. I'm running into similar issues with my go 1.22.1 env. IIRC, the version checking of some of the tools for the build could use a bit a refinement. It would be great to capture some of the specifics so we can make sure to address the root cause.

Any chance you're running Apple silicon? Are you getting clean local test runs? mage -v Test?

Copy link
Member

@troy0820 troy0820 left a comment

Choose a reason for hiding this comment

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

Everything seems to compile, but you have a failing test case.

@kichristensen
Copy link
Contributor Author

@bdegeeter From my point for view no, this PR is only meant to solve a few problems I had when trying to build the project. I had ~3 errors that took a long time to figure out, just because I had Go 1.22 installed. I can remember 2 of them off the top of my head, one was with staticcheck during linting and the other was with controller-gen.
The errors did not mention in anyway that it had something to do with the version of Go.

Thanks for clarifying. I'm running into similar issues with my go 1.22.1 env. IIRC, the version checking of some of the tools for the build could use a bit a refinement. It would be great to capture some of the specifics so we can make sure to address the root cause.

Any chance you're running Apple silicon? Are you getting clean local test runs? mage -v Test?

Unfortunately I'm not running on Applie silicon. I was getting clean test runs before upgrading controller-runtime. I'm currently looking into the failing test

@kichristensen
Copy link
Contributor Author

Everything seems to compile, but you have a failing test case.

I'm looking into the failing test now. I found the root cause to be the breaking change, kubernetes-sigs/controller-runtime#2633 introduced in controller-runtime v0.17.0. But still haven't figured out how to fix it, so any good ideas are more than welcome, but I will continue to looking into the issue

@kichristensen
Copy link
Contributor Author

After some more investigation I think I was wrong in my original assumption that it was kubernetes-sigs/controller-runtime#2633 that triggered the failing test instead I think it is kubernetes-sigs/controller-runtime#2484.
Still investigating how to fix it

@kichristensen
Copy link
Contributor Author

kichristensen commented Apr 1, 2024

@troy0820 All tests are should now be passing

Signed-off-by: Kim Christensen <kimworking@gmail.com>
@troy0820
Copy link
Member

troy0820 commented Apr 1, 2024

I don't see anything else that needs to be done so I'll approve but will not merge it until after the workflows run, etc.

@troy0820 troy0820 merged commit b385a61 into getporter:main Apr 2, 2024
8 checks passed
@kichristensen kichristensen deleted the updateThings branch April 2, 2024 17:55
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.

None yet

3 participants