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 kube vendor dependency and others #286

Merged
merged 1 commit into from May 4, 2020

Conversation

humblec
Copy link
Contributor

@humblec humblec commented Apr 2, 2020

Signed-off-by: Humble Chirammal hchiramm@redhat.com

-->

With kubernetes 1.18 release of client-go, signatures on methods in generated clientsets, dynamic, metadata, and scale clients have been modified to accept context.Context as a first argument. Signatures of Create, Update, and Patch methods have been updated to accept CreateOptions, UpdateOptions and PatchOptions respectively. Signatures of Delete and DeleteCollection methods now accept DeleteOptions by value instead of by reference.  These changes are now accommodated with this PR and client-go and dependencies are updated to v1.18.0

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 2, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @humblec. 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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 2, 2020
@humblec humblec changed the title Update kube vendor dependency and others [WIP] Update kube vendor dependency and others Apr 2, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 2, 2020
@humblec humblec force-pushed the dep-update branch 3 times, most recently from a77b81e to 6e4914b Compare April 3, 2020 18:52
@msau42
Copy link
Collaborator

msau42 commented Apr 3, 2020

/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 Apr 3, 2020
@humblec humblec changed the title [WIP] Update kube vendor dependency and others Update kube vendor dependency and others Apr 4, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 4, 2020
@humblec
Copy link
Contributor Author

humblec commented Apr 4, 2020

/release-note-none

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. 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 Apr 4, 2020
@humblec
Copy link
Contributor Author

humblec commented Apr 4, 2020

@xing-yang @msau42 removing WIP as the CI is passing now. I have changed some function calls too to accomodate params like ctx ..etc to satisfy new invokation of api calls. Please review.

go.mod Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@humblec
Copy link
Contributor Author

humblec commented Apr 7, 2020

@xing-yang looks we are close to get the CI passing , the last errors what I see atm:

...
mkdir -p bin
CGO_ENABLED=0 GOOS=linux go build -mod=vendor -a -ldflags '-X main.version=v2.1.0-4-g4670775a -extldflags "-static"' -o ./bin/csi-snapshotter ./cmd/csi-snapshotter
# github.com/kubernetes-csi/external-snapshotter/v2/pkg/sidecar-controller
pkg/sidecar-controller/snapshot_controller.go:613:89: not enough arguments in call to ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Update
	have (*"github.com/kubernetes-csi/external-snapshotter/v2/pkg/apis/volumesnapshot/v1beta1".VolumeSnapshotContent)
	want (context.Context, *"github.com/kubernetes-csi/external-snapshotter/v2/pkg/apis/volumesnapshot/v1beta1".VolumeSnapshotContent, "k8s.io/apimachinery/pkg/apis/meta/v1".UpdateOptions)
pkg/sidecar-controller/snapshot_controller.go:639:89: not enough arguments in call to ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Update
	have (*"github.com/kubernetes-csi/external-snapshotter/v2/pkg/apis/volumesnapshot/v1beta1".VolumeSnapshotContent)
	want (context.Context, *"github.com/kubernetes-csi/external-snapshotter/v2/pkg/apis/volumesnapshot/v1beta1".VolumeSnapshotContent, "k8s.io/apimachinery/pkg/apis/meta/v1".UpdateOptions)
make: *** [release-tools/build.make:70: build-csi-snapshotter] Error 2

@xing-yang
Copy link
Collaborator

@humblec This looks like the files under v2/pkg are not pointing to files directly under pkg. So I suspect that you missed some steps to remove everything before running the script. Can you follow the README and retry?

rm -fr v2/
ln -sfvn $(pwd) v2
rm -fr pkg/client

@humblec
Copy link
Contributor Author

humblec commented Apr 14, 2020

(as a separate pr)

Sure.. will get on it. Once this is done .. thanks!

@humblec
Copy link
Contributor Author

humblec commented Apr 22, 2020

@xing-yang can I go ahead with the rebase here ?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 23, 2020
@xing-yang
Copy link
Collaborator

@humblec Let's wait until we are done with cherry-pick. Thanks.

@humblec
Copy link
Contributor Author

humblec commented Apr 23, 2020

@humblec Let's wait until we are done with cherry-pick. Thanks.

@xing-yang sure.. meanwhile I was just trying to rebase and get back to shape :)

@humblec
Copy link
Contributor Author

humblec commented Apr 23, 2020

/retest

@Madhu-1
Copy link
Contributor

Madhu-1 commented Apr 29, 2020

@xing-yang will snapshot 2.1.1 includes this PR? will this be part of the next release? if yes when we can expect a release of snapshotter with this change.

@humblec
Copy link
Contributor Author

humblec commented Apr 29, 2020

Rebased to master again @xing-yang.
Make output :

+ CGO_ENABLED=0
+ GOOS=
+ GOARCH=
+ go build -a -ldflags '-X main.version=v2.1.0-23-gdac1c5f9 -extldflags "-static"' -o ./bin/csi-snapshotter ./cmd/csi-snapshotter
[root@localhost external-snapshotter]#

@xing-yang
Copy link
Collaborator

@humblec, we don't need node-api, sample-cli-plugin, and sample-controller in go.mod. I could build without them. Please remove them from go.mod. Other than that, it looks good.

# git diff go.mod
diff --git a/go.mod b/go.mod
index bfb54dcd..e616b80c 100644
--- a/go.mod
+++ b/go.mod
-replace k8s.io/node-api => k8s.io/node-api v0.18.0
-
-replace k8s.io/sample-cli-plugin => k8s.io/sample-cli-plugin v0.18.0
-
-replace k8s.io/sample-controller => k8s.io/sample-controller v0.18.0
-

@humblec
Copy link
Contributor Author

humblec commented May 3, 2020

@humblec, we don't need node-api, sample-cli-plugin, and sample-controller in go.mod. I could build without them. Please remove them from go.mod. Other than that, it looks good.

@xing-yang True, I could build as well, Removed from the patchset. Thanks 👍

@xing-yang
Copy link
Collaborator

Can you squash your commits?

@humblec
Copy link
Contributor Author

humblec commented May 3, 2020

Can you squash your commits?

@xing-yang Done. Only 1 commit at current patchset.

With kubernetes 1.18 release of client-go, signatures on methods in
generated clientsets, dynamic, metadata, and scale clients have been
modified to accept context.Context as a first argument.
Signatures of Create, Update, and Patch methods have been updated
to accept CreateOptions, UpdateOptions and PatchOptions respectively.
Signatures of Delete and DeleteCollection methods now accept
DeleteOptions by value instead of by reference.
These changes are now accommodated with this PR and client-go
and dependencies are updated to v1.18.0

Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
@humblec
Copy link
Contributor Author

humblec commented May 3, 2020

/retest

@xing-yang
Copy link
Collaborator

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: humblec, xing-yang

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 May 4, 2020
@Madhu-1
Copy link
Contributor

Madhu-1 commented May 4, 2020

@xing-yang can you please remove hold label from this PR?

@xing-yang
Copy link
Collaborator

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 4, 2020
@k8s-ci-robot k8s-ci-robot merged commit 859696c into kubernetes-csi:master May 4, 2020
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants