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 dependencies to Kubernetes v1.21.1 #361

Merged
merged 1 commit into from
Jun 3, 2021

Conversation

ash2k
Copy link
Member

@ash2k ash2k commented May 26, 2021

Mostly to drop old Kustomize and other old dependency versions so that this library plays nicer with downstream consumers that want to use more modern versions.

Edit: Switched all go get to go install because go get modifies go.mod/go.sum files while installing a package and go install does not. We want the later beacuse we're not adding the dependency, just installing something to use for code generation/etc. See https://golang.org/doc/go1.16#go-command

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 26, 2021
@k8s-ci-robot k8s-ci-robot requested review from pwittrock and seans3 May 26, 2021 12:08
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 26, 2021
@ash2k ash2k force-pushed the ash2k/update-deps branch 4 times, most recently from 04f154d to 954c2da Compare May 26, 2021 12:57
@ash2k
Copy link
Member Author

ash2k commented May 26, 2021

Build is failing because Go 1.13 is used. I've opened kubernetes/test-infra#22350 to update it.

@mortent
Copy link
Member

mortent commented May 27, 2021

/test cli-utils-presubmit-master

@ash2k ash2k force-pushed the ash2k/update-deps branch from 954c2da to 1828bbe Compare May 27, 2021 00:48
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 2021
@ash2k ash2k force-pushed the ash2k/update-deps branch from 1828bbe to 0c052c2 Compare May 27, 2021 13:03
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 27, 2021
@ash2k ash2k force-pushed the ash2k/update-deps branch 3 times, most recently from 5ecd241 to f51a170 Compare May 27, 2021 13:42
$(GOPATH)/bin/addlicense -y 2020 -c "The Kubernetes Authors." -f LICENSE_TEMPLATE .

verify-license:
(which $(GOPATH)/bin/addlicense || go get github.com/google/addlicense)
(which $(GOPATH)/bin/addlicense || go install github.com/google/addlicense@6d92264d717064f28b32464f0f9693a5b4ef0239) # latest as of 2021-05-27
Copy link
Member Author

Choose a reason for hiding this comment

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

Pinned dependency versions

Makefile Outdated Show resolved Hide resolved
@ash2k ash2k force-pushed the ash2k/update-deps branch 4 times, most recently from 0e4873b to becffbd Compare May 28, 2021 02:15
@ash2k ash2k force-pushed the ash2k/update-deps branch from becffbd to d41f250 Compare May 28, 2021 02:54
@@ -99,7 +99,7 @@ expectedOutputLine "1"
invName=$(kubectl get cm -n test-namespace --selector='cli-utils.sigs.k8s.io/inventory-id' --no-headers | awk '{print $1}')

# There should be one config map that is not the inventory object
kubectl get cm -n test-namespace --selector='!cli-utils.sigs.k8s.io/inventory-id' --no-headers | wc -l > $OUTPUT/status
kubectl get cm -n test-namespace --selector='name=test-config-map-label' --no-headers | wc -l > $OUTPUT/status
Copy link
Member Author

Choose a reason for hiding this comment

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

There is a new ConfigMap kube-root-ca.crt now, so made this more robust.

NAME               DATA   AGE
cm-a               0      2s
cm-b               0      2s
cm-c               0      2s
kube-root-ca.crt   1      7s

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously kind was creating Kubernetes v1.19.1 and now it creates v1.21.1.

k8s.io/cli-runtime v0.21.1
k8s.io/client-go v0.21.1
k8s.io/klog/v2 v2.9.0
k8s.io/kube-openapi v0.0.0-20210421082810-95288971da7e // indirect
Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise it's resolved to an older version, which is incompatible. This version contains the gnostic bump from kubernetes/kube-openapi#220. Kubernetes master uses gnostic 0.5.1 too.

fields := d.GetManagedFields()
Expect(fields[0].Manager).To(Equal("test"))

By("Verify APIService is client-side applied")
By("Verify APIService is server-side applied")
Copy link
Member Author

Choose a reason for hiding this comment

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

1.19.1 didn't support this, 1.21.1 does.

@ash2k
Copy link
Member Author

ash2k commented May 28, 2021

@mortent this is finally ready for review. Cheers.

@tamalsaha
Copy link

Should klog be also updated to klog/v2?

@mortent
Copy link
Member

mortent commented Jun 2, 2021

/lgtm
I'll let @seans3 take a look and do the final approval.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2021
@seans3
Copy link
Contributor

seans3 commented Jun 3, 2021

Should klog be also updated to klog/v2?

You are right. klog is outdated and we should update it to the more recent klog/v2. In fact, I do not think klog is currently working. If we miss some klog -> klog/v2 updates in this PR, we will update them in another PR.

Copy link
Contributor

@seans3 seans3 left a comment

Choose a reason for hiding this comment

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

Thanks for all the work. One small nitpick. I'll merge this quickly once we've added the comments. Let's be on the lookout for version skew issues.

/lgtm

@@ -65,7 +65,7 @@ func serversideApplyTest(c client.Client, invConfig InventoryConfig, inventoryNa
}, apiService)
Expect(err).NotTo(HaveOccurred())
_, found2 := apiService.GetAnnotations()[v1.LastAppliedConfigAnnotation]
Expect(found2).To(Equal(true))
Expect(found2).To(BeFalse())
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a comment that LastAppliedConfigAnnotation is only set for client-side apply, and we've server-side applied here would make this easier for others to follow.

@@ -47,11 +47,11 @@ func serversideApplyTest(c client.Client, invConfig InventoryConfig, inventoryNa
}, &d)
Expect(err).NotTo(HaveOccurred())
_, found := d.ObjectMeta.Annotations[v1.LastAppliedConfigAnnotation]
Expect(found).To(Equal(false))
Expect(found).To(BeFalse())
Copy link
Contributor

Choose a reason for hiding this comment

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

We didn't add sufficient comments initially, but we might want to comment somewhere about the difference of client-side (sets LastAppliedConfigAnnotation) vs. server-side apply (sets field managers) to help explain the tests.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ash2k, seans3

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 Jun 3, 2021
@k8s-ci-robot k8s-ci-robot merged commit 1553e67 into kubernetes-sigs:master Jun 3, 2021
@ash2k ash2k deleted the ash2k/update-deps branch June 3, 2021 05:01
@ash2k
Copy link
Member Author

ash2k commented Jun 3, 2021

@seans3 Thanks for the review. Looks like the bot is buggy and merged it anyway. I'll open a followup with extra comments. #363

@ash2k ash2k mentioned this pull request Jun 3, 2021
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. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants