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

infra: Bump Go to version 1.21.5 #10914

Merged
merged 5 commits into from Jan 5, 2024
Merged

Conversation

brianmcarey
Copy link
Member

@brianmcarey brianmcarey commented Dec 21, 2023

What this PR does / why we need it:

Kubernetes 1.29 is being built with Go 1.21.5[1] - the next release of KubeVirt will be targeting kubernetes 1.29 so we should try to build with Go 1.21.5

[1] kubernetes/kubernetes#122201

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Release note:

KubeVirt is now built with go 1.21.5

Includes the updated builder image

Signed-off-by: Brian Carey <bcarey@redhat.com>
@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/S labels Dec 21, 2023
Signed-off-by: Brian Carey <bcarey@redhat.com>
@brianmcarey brianmcarey force-pushed the go-1.21.5 branch 2 times, most recently from bfc4157 to edd2e7c Compare December 21, 2023 17:03
@@ -1615,10 +1615,10 @@ var _ = SIGMigrationDescribe("VM Live Migration", func() {
By("checking that we were never able to connect")
tlsErrorFound := false
for err := range errors {
if strings.Contains(err.Error(), "remote error: tls: bad certificate") {
if strings.Contains(err.Error(), "remote error: tls: unknown certificate authority") {
Copy link
Member

Choose a reason for hiding this comment

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

This has to change in go implementation. It would be great to reference the commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added this information to the commit - 5cd924b

tlsErrorFound = true
}
Expect(err.Error()).To(Or(ContainSubstring("remote error: tls: bad certificate"), ContainSubstring("EOF")))
Expect(err.Error()).To(Or(ContainSubstring("remote error: tls: unknown certificate authority"), ContainSubstring("EOF")))
Copy link
Member

Choose a reason for hiding this comment

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

Worth adding another Or to support some not-Kubevirt builds.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @xpivarc for spotting that - Added.

The compute-migrations test lane was failing[1] due to a mismatch in TLS
error strings

TLS error handling was improved in go 1.21[2]

[1] https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_kubevirt/10914/pull-kubevirt-e2e-k8s-1.28-sig-compute-migrations/1737851138511736832
[2] golang/go@62a9948

Signed-off-by: Brian Carey <bcarey@redhat.com>
@brianmcarey
Copy link
Member Author

/cc @dhiller @EdDev

Signed-off-by: Brian Carey <bcarey@redhat.com>
@brianmcarey
Copy link
Member Author

/retest

The new builder image hadn't been fully published.

Copy link
Member

@EdDev EdDev left a comment

Choose a reason for hiding this comment

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

Thank you!

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 22, 2023
After the update of golangci-lint - there were still a couple of lint
errors[1]

[1] https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_kubevirt/10914/pull-kubevirt-code-lint/1738136816302690304

Signed-off-by: Brian Carey <bcarey@redhat.com>
@kubevirt-bot kubevirt-bot added size/M and removed lgtm Indicates that a PR is ready to be merged. size/S labels Dec 22, 2023
@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 23, 2023
Copy link
Member

@xpivarc xpivarc left a comment

Choose a reason for hiding this comment

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

/approve

@@ -849,7 +849,8 @@ func (m *InstancetypeMethods) inferDefaultsFromDataVolume(vm *virtv1.VirtualMach
if dvt.Name != dvName {
continue
}
return m.inferDefaultsFromDataVolumeSpec(&dvt.Spec, defaultNameLabel, defaultKindLabel, vm.Namespace)
dvtSpec := dvt.Spec
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was causing the following lint error after the update of golangci-lint

 pkg/instancetype/instancetype.go:852:45: G601: Implicit memory aliasing in for loop. (gosec)
			return m.inferDefaultsFromDataVolumeSpec(&dvt.Spec, defaultNameLabel, defaultKindLabel, vm.Namespace) 

source: https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_kubevirt/10914/pull-kubevirt-code-lint/1738136816302690304

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xpivarc

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 5, 2024
@kubevirt-bot
Copy link
Contributor

@brianmcarey: The following tests 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-kubevirt-check-tests-for-flakes c27f3fe link false /test pull-kubevirt-check-tests-for-flakes
pull-kubevirt-e2e-k8s-1.27-sig-compute c27f3fe link unknown /test pull-kubevirt-e2e-k8s-1.27-sig-compute

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/test-infra repository. I understand the commands that are listed here.

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-bot kubevirt-bot merged commit e443412 into kubevirt:main Jan 5, 2024
36 of 38 checks passed
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. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants