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

Bump golang to 1.23.1 #261

Merged
merged 2 commits into from
Nov 19, 2024
Merged

Bump golang to 1.23.1 #261

merged 2 commits into from
Nov 19, 2024

Conversation

huww98
Copy link
Contributor

@huww98 huww98 commented Oct 29, 2024

external-snapshotter is already using 1.23.1 in go.mod

Bump Golang to 1.23.1

@k8s-ci-robot k8s-ci-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Oct 29, 2024
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 29, 2024
@k8s-ci-robot
Copy link
Contributor

Welcome @huww98!

It looks like this is your first PR to kubernetes-csi/csi-release-tools 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-csi/csi-release-tools has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 29, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @huww98. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 29, 2024
@huww98
Copy link
Contributor Author

huww98 commented Oct 29, 2024

BTW, do you think we should export GOTOOLCHAIN=local in run_with_go? If not, this function is meaningless because go will download the version specified in the go.mod itself. Or should we just remove run_with_go function and rely on go command to manage the version?

My opinion is adding GOTOOLCHAIN=local, so that build will fail if the go version specified in prow.sh is lower than go.mod. This should keep maximum reproducibility of CI result. go command will only upgrade, but not downgrade toolchain.

@huww98
Copy link
Contributor Author

huww98 commented Nov 7, 2024

/cc @xing-yang @jsafrane
PTAL

@jsafrane
Copy link
Contributor

jsafrane commented Nov 8, 2024

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 8, 2024
@jsafrane
Copy link
Contributor

jsafrane commented Nov 8, 2024

BTW, do you think we should export GOTOOLCHAIN=local in run_with_go? If not, this function is meaningless because go will download the version specified in the go.mod itself. Or should we just remove run_with_go function and rely on go command to manage the version?

What approach does the main kubernetes/kubernetes use? Is there anything we could copy?

@huww98
Copy link
Contributor Author

huww98 commented Nov 8, 2024

Maybe we can copy from kubernetes/kubernetes#120279

In short, k/k use:

  • GOTOOLCHAIN=go$(cat .go_version) by default
  • GOTOOLCHAIN=local if FORCE_HOST_GO is set

So maybe such a simple run_with_go should be sufficient:

# Ensure we use the desired version of the Go tools, then run command given as argument.
# Empty parameter uses the already installed Go. In Prow, that version is kept up-to-date by
# bumping the container image regularly.
run_with_go () {
    local version
    version="$1"
    shift
    if [ "$version" ]; then
        version=go$version
    else
        version=local
    fi
   GOTOOLCHAIN=$version run "$@"
}

and it works like this:

$ run_with_go "" go version
2024年11月 9日 星期六 00时59分35秒 CST go1.23.1 /Users/huww98/Downloads$ go version
go version go1.23.1 darwin/arm64

$ run_with_go "1.23.0" go version
2024年11月 9日 星期六 00时59分51秒 CST go1.23.0 /Users/huww98/Downloads$ go version
go version go1.23.0 darwin/arm64

$ run_with_go "1.23.2" go version
go: downloading go1.23.2 (darwin/arm64)
2024年11月 9日 星期六 01时01分21秒 CST go1.23.2 /Users/huww98/Downloads$ go version
go version go1.23.2 darwin/arm64

Please see #262

@pohly
Copy link
Contributor

pohly commented Nov 19, 2024

Manually downloading the right toolchain archive pre-dates the download mechanism in the Go toolchain itself. Nowadays using the toolchain to download the desired version is the right way to go.

@huww98
Copy link
Contributor Author

huww98 commented Nov 19, 2024

Can we get this PR merged? Or should we fix the (seems unrelated) CI failure first?

@jsafrane
Copy link
Contributor

We need the CI to be green before merge to make sure this PR does not break anything.

@jsafrane
Copy link
Contributor

I am staring at the CI logs and they don't make much sense to me.

@pohly
Copy link
Contributor

pohly commented Nov 19, 2024

I suspect it fails because one or both repos are not completely present locally:

--filter=blob:none will filter out all blobs (file contents) until needed by Git

This is used in https://storage.googleapis.com/kubernetes-ci-logs/pr-logs/pull/kubernetes-csi_csi-release-tools/261/pull-kubernetes-csi-release-tools-external-snapshotter/1854867176855441408/clone-log.txt.

I'm not sure yet how to change this - will ask on Slack.

@huww98
Copy link
Contributor Author

huww98 commented Nov 19, 2024

When cloning or fetching from a partial repository (i.e., one itself cloned with --filter), the server-side upload-pack may need to fetch extra objects from its upstream in order to complete the request. By default, upload-pack will refuse to perform such a lazy fetch

https://git-scm.com/docs/git-upload-pack

So I think set GIT_NO_LAZY_FETCH=0 should fix this.

Update: I think I've fixed this in the latest commit. Let's wait for it to turn green.

Without allowing to fetch missing file content, "git subtree pull" fails:

    remote: warning: lazy fetching disabled; some objects may not be available
    remote: fatal: could not fetch b18c53581356c52a220a2baf1c8cf3fd9c57dda6 from promisor remote
    error: git upload-pack: git-pack-objects died with error.
    fatal: git upload-pack: aborting due to possible repository corruption on the remote side.
    remote: aborting due to possible repository corruption on the remote side.�
    fatal: protocol error: bad pack header
pull-test.sh Outdated Show resolved Hide resolved
@jsafrane
Copy link
Contributor

Almost, please also revendor.

@k8s-ci-robot
Copy link
Contributor

@huww98: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubernetes-csi-release-tools-csi-test fd153a9 link false /test pull-kubernetes-csi-release-tools-csi-test

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@huww98
Copy link
Contributor Author

huww98 commented Nov 19, 2024

@pohly @jsafrane Can you approve this PR? So that we can merge these two commits at once.

Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

I think the whole "select Go version" logic needs further work, but in the meantime we can simply bump it.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 19, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: huww98, pohly

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 19, 2024
@k8s-ci-robot k8s-ci-robot merged commit e31de52 into kubernetes-csi:master Nov 19, 2024
5 of 7 checks passed
@huww98
Copy link
Contributor Author

huww98 commented Nov 19, 2024

I think the whole "select Go version" logic needs further work, but in the meantime we can simply bump it.

@pohly for that part, please also help me review #262

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants