search

kubernetes-sigs / cli-utils

This repo contains binaries that built from libraries in cli-runtime.
Apache License 2.0
156 stars 78 forks source link

Applier constructor and run: functional options #454

Closed ash2k closed 2 years ago

ash2k commented 3 years ago

This is a draft for API refactoring using the functional options pattern. The purpose of this PR is to start the conversation on how this can look.

It's easier to look at with the "hide whitespace" diff option enabled.

I suggest to look at this PR in the following order:

  1. pkg/apply/applier.go NewApplier(). See how the new constructor looks.
  2. pkg/apply/applier_options.go. See the new options to configure the applier. New ones may be added later as necessary.
  3. pkg/apply/applier.go Run(). See how the new signature looks.
  4. pkg/apply/applier_run_options.go. See the new options to configure the Run of the applier. New ones may be added later as necessary.
  5. See cmd/* and then tests for usage examples.

TODO:

WDYT @karlkfi @mortent @seans3

ash2k commented 3 years ago

/hold

karlkfi commented 2 years ago

Gotta say, I don't love this pattern. Why would we want to use this over just an Options struct?

Down sides:

  1. It makes it more difficult to see which options are available when using an IDE, which currently shows me all the fields in the Option struct.
  2. It means that anyone can implement a new option struct that satisfies the interface, but Run will just ignore it, right?
  3. Feels less intuitive and more "magic" you need to know to use it.
seans3 commented 2 years ago

Gotta say, I don't love this pattern. Why would we want to use this over just an Options struct?

Down sides:

  1. It makes it more difficult to see which options are available when using an IDE, which currently shows me all the fields in the Option struct.
  2. It means that anyone can implement a new option struct that satisfies the interface, but Run will just ignore it, right?
  3. Feels less intuitive and more "magic" you need to know to use it.

According to Dave Cheney when discussing the Config struct pattern:

It has trouble with defaults, especially if the zero value has a well understood meaning.

Here's the full Functional Options post: https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis

karlkfi commented 2 years ago

I have an alternate suggestion that i think would solve both the defaults issues and still make the API discoverable:

This has the advantages:

Example usage:

applier.Run(ctx, inv, objs, apply.DefaultOptions().
    WithServerSideOptions(r.serverSideOptions).
    WithReconcileTimeout(r.reconcileTimeout).
    WithDryRunStrategy(common.DryRunNone))
ash2k commented 2 years ago

@karlkfi Thanks for your feedback!

It makes it more difficult to see which options are available when using an IDE, which currently shows me all the fields in the Option struct.

I partially agree with this. In practice if we move all/most of the stuff, that shouldn't be public away from the apply package, it will almost exclusively contain relevant options (and the constructor).

It means that anyone can implement a new option struct that satisfies the interface, but Run will just ignore it, right?

Not really. You cannot implement a function to implement the type because it takes a parameter of a private type - see https://github.com/kubernetes-sigs/cli-utils/pull/454/files#diff-44c477dc998e3b6be3729c3697d977a86de5fcd680dd8cee6a181e2d3f94ce9cR35 and https://github.com/kubernetes-sigs/cli-utils/pull/454/files#diff-ff0604c07cd58dd82246f24045636317c9ce34bb516d74c599511042f5e8313bR60.

Overall, this is a well known and widely used pattern in Go. For example, grpc-go uses it. As a user of that library, I think it's quite good in terms of UX.

Feels less intuitive and more "magic" you need to know to use it.

It depends if you have used it before. As I said above, it's a widely known and used pattern in Go. If you have worked with it before, you'll feel right at home.

I have an alternate suggestion

To me as a user that reminds Java's builders :) I think this is fine, but less commonly used in Go.

defaults are always set, don't need to match the zero value in Go, and don't always need to be specified by the user.

Same as with functional options.

all possible options are discoverable using IDE auto-complete on the options struct.

This is good, but with func. options it's package name (e.g. apply) vs the private struct. Autocomplete works on both, but with functional options you get a bit of unrelated stuff too (depends on the IDE, I guess).

I think both approaches are better than what we have now. As I user, I personally would prefer functional options because it's more idiomatic in Go, but it's just me. 🤷

karlkfi commented 2 years ago

How is the functional options pattern more idiomatic than the builder pattern?

I don't really buy that Functional Options is idiomatic just because Dave Cheney made a blog about it. And just because Builder pattern is popular in Java doesn't mean it's bad or not idiomatic Go. These seem like spurious arguments.

Consistency with other big projects is mildly compelling, but would be more compelling if it were something more closely related, like K8s itself. It's still sort of an appeal to authority tho.

I can sort of get on board with functional options being better than the existing options struct, but I see it more as a tradeoff than a solid win. And I'm not convinced it's better than the build pattern.

That said, none of this seems like a huge priority. Even if it's an improvement, it's still an API change that's going to require refactoring all the existing clients, without any obvious return on investment for existing users.

seans3 commented 2 years ago

How is the functional options pattern more idiomatic than the builder pattern?

I don't really buy that Functional Options is idiomatic just because Dave Cheney made a blog about it. And just because Builder pattern is popular in Java doesn't mean it's bad or not idiomatic Go. These seem like spurious arguments.

Consistency with other big projects is mildly compelling, but would be more compelling if it were something more closely related, like K8s itself. It's still sort of an appeal to authority tho.

I can sort of get on board with functional options being better than the existing options struct, but I see it more as a tradeoff than a solid win. And I'm not convinced it's better than the build pattern.

That said, none of this seems like a huge priority. Even if it's an improvement, it's still an API change that's going to require refactoring all the existing clients, without any obvious return on investment for existing users.

Either builder, functional options, options struct, or a hybrid, we are going to simplify the API. The return on investment for the two current clients are that they will have a stable API. The two current existing clients are going to have to change no matter what, because we are certainly going to simplify the API to make it more accessible to other clients. The API is not even alpha yet. It is much better to change it now that there are only two known clients, instead of trying to update it later when there may be more. Specifically, the simplest default API will not assume a custom inventory object. In fact, the simplest case will not activate pruning functionality at all. The simplest case will be to merely activate the wrapped kubectl apply functionality--no pruning, depends on, or apply-time mutation. And the API will support this as the default. The SIG CLI refactored dependencies literally for years by moving the code base into staging, which finally allows kubectl code reuse. And this open source effort in a SIG CLI repository is one of the first known reuses of kubectl apply. The open source community deserves a simple, accessible API to this library, and it will get it.

k8s-ci-robot commented 2 years ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ash2k To complete the pull request process, please assign liujingfang1 after the PR has been reviewed. You can assign the PR to them by writing /assign @liujingfang1 in a comment when ready.

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

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/kubernetes-sigs/cli-utils/blob/master/OWNERS)** 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 commented 2 years ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ash2k To complete the pull request process, please assign liujingfang1 after the PR has been reviewed. You can assign the PR to them by writing /assign @liujingfang1 in a comment when ready.

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

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/kubernetes-sigs/cli-utils/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
ash2k commented 2 years ago

/kind cleanup

ash2k commented 2 years ago

I've opened https://github.com/kubernetes-sigs/cli-utils/pull/485 with an alternative approach with the builder pattern.

k8s-ci-robot commented 2 years ago

@ash2k: PR needs rebase.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.