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.

Sign up for GitHub

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

Jump to bottom

Cherry pick of #287: Updated sidecar to not require VolumeSnapshotClass for snapshot deletion #466

Merged
merged 1 commit into from
Feb 11, 2021
Merged

Cherry pick of #287: Updated sidecar to not require VolumeSnapshotClass for snapshot deletion #466

merged 1 commit into from
Feb 11, 2021

Conversation

ialidzhikov
Copy link
Contributor

@ialidzhikov ialidzhikov commented Feb 2, 2021 •
edited by xing-yang
Loading

/kind bug

Cherry pick of #287 on release-2.1.

#287: Updated sidecar to not require VolumeSnapshotClass for snapshot deletion

Which issue(s) this PR fixes:

Ref #306

Does this PR introduce a user-facing change?:

Cherry pick of #287: Allows the sidecar to delete volume snapshots if the volume snapshot class is not found.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 2, 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.

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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 2, 2021
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 2, 2021
@ialidzhikov
Copy link
Contributor Author

/cc @huffmanca @xing-yang

@ialidzhikov ialidzhikov mentioned this pull request Feb 2, 2021
Closed
@xing-yang
Copy link
Collaborator

/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 Feb 4, 2021
@ialidzhikov
Copy link
Contributor Author

/test pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20

@ialidzhikov
Copy link
Contributor Author

@xing-yang , pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20 fails to clone v1.20.0 of kubernetes/kubernetes. I don’t think that it is something related to this PR.

@ialidzhikov
Copy link
Contributor Author

/assign @xing-yang

@xing-yang
Copy link
Collaborator

/test pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20

@ialidzhikov
Copy link
Contributor Author

@xing-yang , I believe the job configuration is wrong.

It currently tries to clone 1.20.0 but there is no such tag. It should be v1.20.0.

-git clone --single-branch --branch 1.20.0 https://github.com/kubernetes/kubernetes /tmp/kubernetes
+git clone --single-branch --branch v1.20.0 https://github.com/kubernetes/kubernetes /tmp/kubernetes

Where lives the configuration this job?

@xing-yang
Copy link
Collaborator

@pohly any idea why "v" is missing from "v1.20.0" for this backport? It is there for PRs running in master branch.

@xing-yang
Copy link
Collaborator

xing-yang commented Feb 8, 2021 •
edited
Loading

I see a difference between the two:

https://github.com/kubernetes-csi/external-snapshotter/blob/release-2.1/release-tools/prow.sh#L573
https://github.com/kubernetes-csi/external-snapshotter/blob/master/release-tools/prow.sh#L575

https://github.com/kubernetes-csi/external-snapshotter/blob/master/release-tools/prow.sh#L68-L78

@ialidzhikov
Copy link
Contributor Author

I see a difference between the two:

https://github.com/kubernetes-csi/external-snapshotter/blob/release-2.1/release-tools/prow.sh#L573
https://github.com/kubernetes-csi/external-snapshotter/blob/master/release-tools/prow.sh#L575

https://github.com/kubernetes-csi/external-snapshotter/blob/master/release-tools/prow.sh#L68-L78

See https://github.com/kubernetes-csi/external-snapshotter/pull/464/files -> version_to_git func. I believe somewhere in https://github.com/kubernetes-csi/external-snapshotter/pull/464/files there is a fix for the failing git clone in pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20..

@ialidzhikov ialidzhikov force-pushed the cherrypick/do-not-require-class branch from d7ba894 to b436899 Compare February 8, 2021 16:39
@xing-yang
Copy link
Collaborator

I think you can submit a separate PR just to add that version_to_git logic in release-2.1.

CC @pohly @mattcary

@ialidzhikov
Copy link
Contributor Author

The PR was based on old revision from release-2.1 branch. I rebased it to latest release-2.1 but it failed with the same error. @xing-yang , I have locally a change that can fix the git clone for pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20 . Should I create a PR with it to tackle this issue or? I see that this job/check is not labelled as required.

@xing-yang
Copy link
Collaborator

We can't merge a PR that can't pass a CI.

@ialidzhikov
Copy link
Contributor Author

ialidzhikov commented Feb 8, 2021 •
edited
Loading

@xing-yang , I created kubernetes/test-infra#20792 which should address the issue with the K8s 1.20 tests that should not be executed against [email protected].

@pohly
Copy link
Contributor

pohly commented Feb 9, 2021

We can't merge a PR that can't pass a CI.

1-20-on-kubernetes-1-20 is intentionally not required to pass at this time, therefore you can merge PRs even when it is failing.

@ialidzhikov ialidzhikov mentioned this pull request Feb 9, 2021
Closed
@ialidzhikov ialidzhikov force-pushed the cherrypick/do-not-require-class branch from b436899 to 463484f Compare February 9, 2021 13:38
vpnachev
vpnachev approved these changes Feb 10, 2021
View reviewed changes
Copy link

@vpnachev vpnachev left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot
Copy link
Contributor

@vpnachev: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ialidzhikov, rfranzke, vpnachev, 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 Feb 11, 2021
@k8s-ci-robot k8s-ci-robot merged commit 3ca7ef3 into kubernetes-csi:release-2.1 Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rfranzke rfranzke rfranzke approved these changes

@vpnachev vpnachev vpnachev approved these changes

@jingxu97 jingxu97 Awaiting requested review from jingxu97

@saad-ali saad-ali Awaiting requested review from saad-ali

@huffmanca huffmanca Awaiting requested review from huffmanca

@xing-yang xing-yang Awaiting requested review from xing-yang

Assignees

@xing-yang xing-yang

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/bug Categorizes issue or PR as related to a bug. 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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ialidzhikov @k8s-ci-robot @xing-yang @pohly @vpnachev @rfranzke @huffmanca