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

add ResourceVersion to DeleteOptions.Preconditions #74040

Merged
merged 9 commits into from Mar 12, 2019

Conversation

@ajatprabha
Copy link
Contributor

commented Feb 13, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test

/kind feature

/kind flake

What this PR does / why we need it:
This PR adds ResourceVersion as a precondition for delete in order to ensure a delete fails if an unobserved change happens to an object.
Which issue(s) this PR fixes:

Fixes #73648

Does this PR introduce a user-facing change?:

Add ResourceVersion as a precondition for delete in order to ensure a delete fails if an unobserved change happens to an object.
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

Hi @ajatprabha. Thanks for your PR.

I'm waiting for a kubernetes 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.

@ajatprabha

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

/assign @lavalamp

@lavalamp

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

/ok-to-test

/assign @jennybuckley

@jennybuckley
Copy link
Contributor

left a comment

This probably needs a release note since it changes the API
@kubernetes/api-reviewers

@ajatprabha

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

I need help with failing Integration tests at etcd_storage_path_test.go:121. I tried a few ways to fix with no luck.

defer func() {
if !t.Failed() { // do not cleanup if test has already failed since we may need things in the etcd dump
if err := client.cleanup(all); err != nil {
t.Fatalf("failed to clean up etcd: %#v", err)
}
}
}()
func (c *allClient) cleanup(all *[]cleanupData) error {
for i := len(*all) - 1; i >= 0; i-- { // delete in reverse order in case creation order mattered
obj := (*all)[i].obj
gvr := (*all)[i].resource
if err := c.dynamicClient.Resource(gvr).Namespace(obj.GetNamespace()).Delete(obj.GetName(), nil); err != nil {
return err
}
}
return nil
}
cc: @liggitt @lavalamp @jennybuckley

@ajatprabha ajatprabha force-pushed the ajatprabha:issue_73648 branch from 1e151fd to 40d2a5b Feb 16, 2019

@fejta-bot

This comment has been minimized.

Copy link

commented Feb 21, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@lavalamp
Copy link
Member

left a comment

This looks basically ready to go, thanks for the change! After API reviewer sign off (I would do it here but there's a new process) this should merge and then I think the last thing to do is a documentation update.

@lavalamp

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

I meant to lgtm/approve this right after last API Machinery SIG meeting and (clearly) it didn't happen. Now it is code freeze so we need some sort of exception I'm sure :(

My schedule today is... full, so I'm not going to have a chance to look up the process. @ajatprabha if you have time to do that, I'm happy to sponsor or vouch for the change if that's a part of the process.

@ajatprabha ajatprabha force-pushed the ajatprabha:issue_73648 branch from 0c3c2b2 to e72379c Mar 11, 2019

@ajatprabha ajatprabha force-pushed the ajatprabha:issue_73648 branch from e72379c to 498034c Mar 11, 2019

@ajatprabha

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

/test pull-kubernetes-godeps

@ajatprabha

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

/test pull-kubernetes-e2e-gce

ajatprabha added some commits Feb 13, 2019

@ajatprabha ajatprabha force-pushed the ajatprabha:issue_73648 branch from 498034c to 4bd0c4a Mar 12, 2019

@nikopen

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

/milestone v1.14

@k8s-ci-robot k8s-ci-robot added this to the v1.14 milestone Mar 12, 2019

@lavalamp

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

/approve
/lgtm

Thanks for the change!

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 12, 2019

@lavalamp

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

(Just FYI, I would ask for a squash but I don't want to risk another rebase...)

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ajatprabha, lavalamp

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

@lavalamp lavalamp moved this from Assigned to In progress in API Reviews Mar 12, 2019

@k8s-ci-robot k8s-ci-robot merged commit cc8afb2 into kubernetes:master Mar 12, 2019

17 checks passed

cla/linuxfoundation ajatprabha authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@ajatprabha ajatprabha deleted the ajatprabha:issue_73648 branch Mar 26, 2019

@liggitt liggitt removed this from In progress in API Reviews Mar 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.