Add options to client.Delete()

[grantr]

Allows Delete requests to carry options like GracePeriodSeconds, Preconditions, and PropagationPolicy. I chose the functional options pattern here so the function signature remains the same for all existing callsites.

To use the default options, call Delete with the same signature as before:

client.Delete(context.TODO(), obj)

This Delete call sets a GracePeriodSeconds for the request:

client.Delete(context.TODO(), obj, client.GracePeriodSeconds(1))

Multiple options can be set:

client.Delete(context.TODO(), obj,
  client.GracePeriodSeconds(1),
  client.PropagationPolicy(metav1.DeletePropagationForeground))

I wrote tests for the DeleteOptions struct, but I wasn't sure how to test the actual request content. With the current testing strategy, we can only test behavior by inspecting what the apiserver did, and in this case that's fairly subtle since it involves timeouts and cascading deletes. It would be easier to use httptest or a similar mock to inspect the request that's sent by the client.

Fixes #76.

[DirectXMan12]

Our list options are chained instead of varargs. I'm not necessary objecting to the varargs approach (I like how it ends up looking, but I think there are merits to both), but I think we need to be consistent.

[grantr]

In general, the benefit of functional options is that they're invisible until you need them. This seems more useful for Delete than List. I expect Delete options will be rarely used, while almost all List calls will use options. The two operations have different ergonomic needs.

I can create another PR exploring functional ListOptions if that seems useful.

[DirectXMan12]

In general, the benefit of functional options is that they're invisible until you need them.

Agreed, I do like that aspect.

I can create another PR

yeah, I think we should have a common experience. Please do create a separate PR for varargs list options.

[DirectXMan12]

generally looks good. I would rewrite the tests to capture desired behavior from a user perspective though, instead of being broken out into the smallest possible units.

For example, if I were describing this, I would say

• it should allow setting GracePeriodSeconds
• it should allow setting Precondition
• it should allow setting PropogationPolicy
• it should produce empty metav1.DeletionOptions if nil
• it should be possible to merge multiple options together

For each of those first three, I'd start with the function that produces the desired flag, call apply on it, and then check that the output metav1.ListOptions was as expected. We still test the same set of functionality, but it's broken up into more logical sequences from the perspective of how we'd expect someone to use it.

[droot]

@grantr will wait for your second PR to compare how it looks.

Also, another major factor to consider is that the changes should be backward compatible. Implementation in current PR meets that criterion.

[grantr]

@droot @DirectXMan12 #106 converts client.List to use functional list options also. It's not backward compatible, but it's close for the common case of a single package-scoped helper function:

// current
List(ctx, client.InNamespace(ns), list)
// proposed
List(ctx, list, client.InNamespace(ns))

More examples in #106.

[grantr]

I would rewrite the tests to capture desired behavior from a user perspective though, instead of being broken out into the smallest possible units.

The list you propose is all the existing tests less one! 😜I folded the metav1.DeleteOptions test into the others and refactored a bit. Take a look; let me know what you think.

[DirectXMan12]

tests look good. will do one more pass tonight or tomorrow morning, then merge.

[DirectXMan12]

/lgtm
/approve

[DirectXMan12]

whoops, looks like you missed a fake implementation in one of the tests.

[grantr]

Oops, should have caught that earlier. I updated the fake client interface, but didn't actually implement any delete options. I added a TODO to implement delete propagation.

This brings up a subtlety of backward compatibility for this change: usage of the client interface (Delete() calls) is backward compatible, but implementation of the client interface is not. The only realistic scenario I can think of is that client mocks used in other projects might need to be updated.

[DirectXMan12]

Still technically a incompatible change then, I think. We'll do it once everything's migrated to the pinned deps.

[grantr]

Can this change also go forward with #106 (comment)?

[DirectXMan12]

Yep, this can go forward now.

[DirectXMan12]

Fix golint issues (some missing godocs), and this is good to go.

[grantr]

Fixed the golint issues and rebased into a single commit.

[grantr]

Looks like the gosec check is failing, but it doesn't seem to be anything related to the changes in this PR.

[DirectXMan12]

there's an open PR about gosec

[grantr]

All good now.

[DirectXMan12]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, grantr

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:

• OWNERS [DirectXMan12]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment