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

build: update rook and k8s api version #285

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

subhamkrai
Copy link
Collaborator

this commit updates rook and k8s api version

Description of your changes:

Which issue is resolved by this Pull Request:
Resolves #

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Reviewed the developer guide on Submitting a Pull Request
  • CI tests has been updated, if necessary.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.

go.mod Outdated
go 1.21
go 1.22.0

toolchain go1.22.1
Copy link
Member

Choose a reason for hiding this comment

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

Why was the toochain needed as a separate version from go above? It's nice if we can just declare 1.22 and it automatically picks up the latest patch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's discuss this in huddle,
the k8s 0.30 api are bringing those changes either we have to mention toolchain go.* or we have to use explicit version of go in go.mod like go 1.22.1

go.mod Outdated
@@ -1,19 +1,21 @@
module github.com/rook/kubectl-rook-ceph

go 1.21
go 1.22.0
Copy link
Member

Choose a reason for hiding this comment

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

lets use 1.22 not stick to x.y.z so that we always use latest released version and also we will get build problem in downstream if we use x.y.z here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Madhu-1 if we use go 1.22 then we need to use toolchain go1.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.

Copy link
Member

Choose a reason for hiding this comment

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

toolchain is automatically picked up based on the version right? i think that should be fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes it is auto picked by running go mod tidy

Copy link
Member

@travisn travisn Apr 24, 2024

Choose a reason for hiding this comment

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

The go toolchain doc says:
"That is, the go line sets the minimum required Go version necessary to use a module or workspace."

Sounds fine to have 1.22.0 as the min version. But do we need to be opinionated about the toolchain that is chosen? Can we remove the toolchain line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we can remove the toolchain we just need to use the same go toolchain version go *. See the changes now

Copy link
Member

Choose a reason for hiding this comment

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

we will get to problems like red-hat-storage/odf-multicluster-orchestrator#197, sadly that PR doesn't contain details but that was done to fix the build issue in Downstream, for some reason we don't get x.y.z specific go version in downstream , and also this doesn't allow developers to use lower version of 1.22 to build the repo? just ensure that we are covering all the cases

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we can discuss this in today's meeting, but anyways we need to use specific version in go.mod either with/without toolchain

go 1.22.0

toolchain go1.22.1

is added when ran go mod tidy

Copy link
Member

Choose a reason for hiding this comment

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

This does allow multiple versions of Go to be used locally at least during development. If I build this branch then run go version I see:

% go version
go version go1.22.1 darwin/arm64

Which is interesting because I don't recall installing 1.22, it must have pulled it automatically. Then if I go back to the rook repo, I still see go 1.21:

% go version
go version go1.21.5 darwin/arm64

So the main requirement I assume is that the downstream build would need to use 1.22 for this repo

this commit updates rook and k8s api version

Signed-off-by: subhamkrai <srai@redhat.com>
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