dry-run: Add resttests #67136
dry-run: Add resttests #67136
Conversation
k8s-ci-robot
added
release-note-none
| @@ -1005,7 +1005,7 @@ func (e *Store) Delete(ctx context.Context, name string, options *metav1.DeleteO | |||
| } | |||
|
|
|||
| // If dry-run, then just return the object as just saved. | |||
| if dryrun.IsDryRun(options.DryRun) { | |||
| if dryrun.IsDryRun(options.DryRun) && out != nil { | |||
There was a problem hiding this comment.
I didn't expect this. Why shouldn't dry-run be honored in all cases? It seems like even an error would be better than actually doing a delete when a dry run is requested.
There was a problem hiding this comment.
I am also confused about why we need this if statement when we also pass something about dry run into the delete statement below.
There was a problem hiding this comment.
The code before that condition/early-return is doing a guaranteedupdate to add the finalizer and deletion timestamp. That guaranteed update can be dry-run, which means that the object is never persisted with deletion timestamp and finalizers.
The code after does the delete when an immediate delete is required. Actually it does an atomic get-and-delete and returns the object as it is before being deleted (including finalizer and deletion timestamp). The dry-run delete just does a get, but in this case, it doesn't get the proper object since the guaranteed update never really happened.
The goal of this condition is just to avoid the second "get", but to return the object with the finalizer and deletion-timestamp.
There was a problem hiding this comment.
Great, can you state this in a comment in the code? It's not obvious.
| @@ -749,6 +823,38 @@ func (t *Tester) testUpdateIgnoreClusterName(obj runtime.Object, createFn Create | |||
| // ============================================================================= | |||
| // Deletion tests. | |||
|
|
|||
| func (t *Tester) testDeleteDryRunNoGraceful(obj runtime.Object, createFn CreateFunc, getFn GetFunc, isNotFoundFn IsErrorFunc) { | |||
There was a problem hiding this comment.
Why does dry run deletion need its own function?
There was a problem hiding this comment.
Because the non-dry run version verifies that the object has been removed (this one should probably verify that the object still exists)
There was a problem hiding this comment.
Would we end up with easier to maintain code if they checked opts before doing that, rather than duplicating the functions?
There was a problem hiding this comment.
I've changed it because I don't have a strong opinion. Sometimes it's easier to maintain mostly-copy-pasted code than factorizing diverging code with lots of if-statements. In this case it's just one if-statement so I don't really care :-)
|
/sig api-machinery |
k8s-ci-robot
added
the
sig/api-machinery
|
/uncc |
| createdMeta.SetCreationTimestamp(createdFakeMeta.GetCreationTimestamp()) | ||
| createdFakeMeta.SetResourceVersion("") | ||
| createdMeta.SetResourceVersion("") | ||
| // TODO(apelisse): This is wrong, we should have empty UID. |
| ctx := t.TestContext() | ||
|
|
||
| foo := obj.DeepCopyObject() | ||
| t.setObjectMeta(foo, t.namer(1)) | ||
| if err := createFn(ctx, foo); err != nil { | ||
| t.Errorf("unexpected error: %v", err) | ||
| } | ||
| defer t.delete(ctx, foo) |
There was a problem hiding this comment.
I'm confused about this-- if not dryRun, it should have already been deleted at the end, and if dryRun it should not be removed at all? What am I missing?
apelisse
force-pushed
the
dry-run-tests
branch
from
3622fdc
to
f3b8e85
Compare
|
/retest |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: apelisse, 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 |
k8s-ci-robot
added
the
approved
|
/retest Review the full test history for this PR. Silence the bot with an |
|
Automatic merge from submit-queue (batch tested with PRs 66177, 66185, 67136, 67157, 65065). If you want to cherry-pick this change to another branch, please follow the instructions here. |
Add Resttests
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Partially #67128
Special notes for your reviewer:
Release note: