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

Bump Go version to 1.22 & controller-gen to 0.14 #1437

Merged
merged 10 commits into from
Mar 22, 2024
Merged

Bump Go version to 1.22 & controller-gen to 0.14 #1437

merged 10 commits into from
Mar 22, 2024

Conversation

roothorp
Copy link
Collaborator

All Submissions:

  • Have you signed our CLA?
  • Put closes #XXXX in your comment to auto-close the issue that your PR fixes (if there is one).
  • Update docs/release-notes/release-notes.md if your changes should be included in the release notes for the next release.

@josvazg
Copy link
Collaborator

josvazg commented Mar 12, 2024

You need to run make licenses.csv

@josvazg
Copy link
Collaborator

josvazg commented Mar 12, 2024

It seems you might also need to update controller-gen as well.

@roothorp roothorp added the cloud-tests Run expensive Cloud Tests: Integration & E2E label Mar 12, 2024
@roothorp roothorp changed the title Bump Go version to 1.22 Bump Go version to 1.22 & controller-gen to 0.14 Mar 12, 2024
@roothorp
Copy link
Collaborator Author

I think the e2e tests are getting a false cache hit (because we haven't actually changed any dependencies) and are still using 1.21 :(

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 12, 2024 •
edited
Loading

https://app.codecov.io/github/mongodb/mongodb-atlas-kubernetes/commit/63efd6e16e62cf78cc30b5f413f0f30872a40681

@roothorp roothorp added cloud-tests Run expensive Cloud Tests: Integration & E2E and removed cloud-tests Run expensive Cloud Tests: Integration & E2E labels Mar 13, 2024
igor-karpukhin
igor-karpukhin reviewed Mar 13, 2024
View reviewed changes
Makefile
@@ -113,7 +113,7 @@ help: ## Show this help screen
all: manager ## Build all binaries

go-licenses:
@which go-licenses || go install github.com/google/go-licenses@latest
go install github.com/google/go-licenses@latest
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is going to always try to install go-licences, isn't it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah (the same with controller-gen) but it ensures we're installing the version we want, and as far as I can tell go install will just exit if it's already installed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to expand; this feels safer, and I had issues when doing this with both controller-gen and go-licenses because I had old versions installed and which doesn't care what version there is, just that it exists.

igor-karpukhin
igor-karpukhin reviewed Mar 13, 2024
View reviewed changes
Makefile
@@ -239,7 +239,7 @@ vet: $(TIMESTAMPS_DIR)/vet ## Run go vet against code

.PHONY: controller-gen
controller-gen: ## Download controller-gen locally if necessary
@which controller-gen || go install sigs.k8s.io/controller-tools/cmd/controller-gen@v0.9.2
go install sigs.k8s.io/controller-tools/cmd/controller-gen@v0.14.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

...here as well

igor-karpukhin
igor-karpukhin reviewed Mar 13, 2024
View reviewed changes
Copy link
Collaborator

@igor-karpukhin igor-karpukhin left a comment

Choose a reason for hiding this comment

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

LGTM in general. Just two questions about the Makefile targets

roothorp
roothorp commented Mar 13, 2024
View reviewed changes
go.mod
@@ -1,6 +1,6 @@
module github.com/mongodb/mongodb-atlas-kubernetes/v2

go 1.21
go 1.22.1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is important to note, and is discussed at some length in golang/go#62278.

TL;DR - the go in go.mod expects a 'full' version (maj.min.patch), and has since 1.21. You can define a maj.min, but then you need to define toolchain in go.mod as well. This feels like a breaking change 🤷, but I think we managed to fluke avoiding this issue with 1.21 because there was an actual (development!) release called 1.21.

Going forward we should define the patch version here too (which requires more frequent updates). Note that this doesn't (currently) apply to the Docker bases we use, where there is still a generic 1.22 version that gets bumped to whatever the newest patch version is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, I realized this as well in atlas CLI :-/

@roothorp roothorp added cloud-tests Run expensive Cloud Tests: Integration & E2E and removed cloud-tests Run expensive Cloud Tests: Integration & E2E labels Mar 13, 2024
@roothorp roothorp added cloud-tests Run expensive Cloud Tests: Integration & E2E and removed cloud-tests Run expensive Cloud Tests: Integration & E2E labels Mar 13, 2024
@roothorp roothorp added cloud-tests Run expensive Cloud Tests: Integration & E2E and removed cloud-tests Run expensive Cloud Tests: Integration & E2E labels Mar 13, 2024
@josvazg
Copy link
Collaborator

josvazg commented Mar 15, 2024

This will need rebase now before merging, but hopefully it will get simpler after the few fixes in the CI.

helderjs
helderjs requested changes Mar 18, 2024
View reviewed changes
.github/workflows/test-int.yml Outdated Show resolved Hide resolved
tools/clean/go.mod Outdated Show resolved Hide resolved
@roothorp roothorp added cloud-tests Run expensive Cloud Tests: Integration & E2E and removed cloud-tests Run expensive Cloud Tests: Integration & E2E labels Mar 19, 2024
@josvazg
Copy link
Collaborator

josvazg commented Mar 20, 2024

We need to run make recompute-licenses and update. I would do that after rebasing from the merge of the release, rerun all suite and merge.

@roothorp roothorp requested a review from helderjs March 21, 2024 13:43
@roothorp roothorp removed the cloud-tests Run expensive Cloud Tests: Integration & E2E label Mar 21, 2024
helderjs
helderjs requested changes Mar 21, 2024
View reviewed changes
.github/workflows/test-e2e-gov.yml Outdated Show resolved Hide resolved
s-urbaniak
s-urbaniak approved these changes Mar 22, 2024
View reviewed changes
Copy link
Collaborator

@s-urbaniak s-urbaniak left a comment

Choose a reason for hiding this comment

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

great work, LGTM 👍

@roothorp roothorp merged commit 065d6f0 into main Mar 22, 2024
46 checks passed
@roothorp roothorp deleted the bump-go-122 branch September 26, 2024 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josvazg josvazg josvazg approved these changes

@helderjs helderjs helderjs approved these changes

@s-urbaniak s-urbaniak s-urbaniak approved these changes

@igor-karpukhin igor-karpukhin igor-karpukhin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@roothorp @josvazg @helderjs @s-urbaniak @igor-karpukhin