Skip to content

Conversation

@ialidzhikov
Copy link
Contributor

/kind cleanup

Fixes #603

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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 May 3, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @ialidzhikov. 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.

Details

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 requested review from chrishenzie and msau42 May 3, 2021 17:05
@ialidzhikov
Copy link
Contributor Author

/assign @msau42

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 3, 2021
@pohly
Copy link
Contributor

pohly commented May 7, 2021

/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 May 7, 2021

replace k8s.io/component-helpers => k8s.io/component-helpers v0.21.0

replace k8s.io/mount-utils => k8s.io/mount-utils v0.21.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't remove these replace statements.

go get -u ./... breaks without them.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can prune them by running release-tools/go-get-kubernetes.sh 1.21.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, to be honest I am not super convinced that we should keep the replace statements. I now executed release-tools/go-get-kubernetes.sh 1.21.0 and the end result was pretty much the same - it added the same replace statements.
I see replace statements for pkgs such as k8s.io/sample-apiserver, k8s.io/sample-cli-plugin, k8s.io/sample-controller which are obviously pkgs that the external-provisioner does not vendor or import in any way. I don't see reason why we should have such replace statements.
To my understanding the main goal of #603 is actually to get rid of these replace statements.
So I don't think release-tools/go-get-kubernetes.sh is needed/useful anymore as external-provisioner no longer vendors k8s.io/kubernetes. If you want I can put replace statements only for the k8s.io pkgs that external-provisioner vendors and imports.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, to be honest I am not super convinced that we should keep the replace statements.

Then try running go get -u ./... in your branch without them...

I now executed release-tools/go-get-kubernetes.sh 1.21.0 and the end result was pretty much the same

Ah, sorry. I thought we had updated release-tools/go-get-kubernetes.sh already, but it is still behind the one in csi-release-tools. Try the script from https://github.com/kubernetes-csi/csi-release-tools/blob/master/go-get-kubernetes.sh - that one will prune unused replace statements.

To my understanding the main goal of #603 is actually to get rid of these replace statements.

No, the main goal is to replace code from k/k with code that is meant to be used by out-of-tree components.

If you want I can put replace statements only for the k8s.io pkgs that external-provisioner vendors and imports.

That's what the updated go-get-kubernetes.sh does.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, it actually works better than expected. But elsewhere I've seen it update client-go to a 1.x version that was older than the intended 0.21.0, so I stand by my assessment that these replace statements are a useful safeguard against using inconsistent and potentially wrong versions of the Kubernetes packages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this particular issue with client-go was fixed in https://github.com/kubernetes/client-go/releases/tag/v1.5.2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry. I thought we had updated release-tools/go-get-kubernetes.sh already, but it is still behind the one in csi-release-tools. Try the script from https://github.com/kubernetes-csi/csi-release-tools/blob/master/go-get-kubernetes.sh - that one will prune unused replace statements.

Thanks for the hint! The run with the latest release-tools/go-get-kubernetes.sh script was much better. I updated the PR now with https://github.com/kubernetes-csi/external-provisioner/compare/a44abfbfb181d32d7c47d048cdf320c1efee0420..6f779f3c7eb8082c534aada2a95d796eaac8ce23.

PS. However the output from release-tools/go-get-kubernetes.sh 1.21.0 was still not the one that I expected. It updated some of the dependencies to v0.21.1-rc.0.

$ release-tools/go-get-kubernetes.sh 1.21.0
# [...]
go get: upgraded k8s.io/apimachinery v0.21.0 => v0.21.1-rc.0
SUCCESS

From the output I see that the script fetches the kubernetes-1.21.0 branch instead of the corresponding tag that I want. However this is just a guess and the small issue with the script is a separate topic for enhancement.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that the client-go issue seems to be resolved, I prefer to remove the replace statements to keep this easier to comprehend and update.

This is fine for now

Copy link
Contributor

Choose a reason for hiding this comment

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

However the output from release-tools/go-get-kubernetes.sh 1.21.0 was still not the one that I expected. It updated some of the dependencies to v0.21.1-rc.0.

Go does that when both tags are identical. It then reports the more recent one.

@ialidzhikov ialidzhikov force-pushed the cleanup/dependency branch from a44abfb to 6f779f3 Compare May 7, 2021 14:01
@k8s-ci-robot
Copy link
Contributor

@ialidzhikov: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-csi-external-provisioner-1-21-on-kubernetes-1-21 6f779f3 link /test pull-kubernetes-csi-external-provisioner-1-21-on-kubernetes-1-21

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.

Details

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.

@msau42
Copy link
Collaborator

msau42 commented May 7, 2021

/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 7, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ialidzhikov, msau42

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

The pull request process is described here

Details 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 7, 2021
@k8s-ci-robot k8s-ci-robot merged commit a9b470d into kubernetes-csi:master May 7, 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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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-none Denotes a PR that doesn't merit a release note. 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.

Remove k8s.io/kubernetes dependency

4 participants