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

shortcuts for Api::apply and Api::apply_status #649

Open
clux opened this issue Oct 10, 2021 · 2 comments
Open

shortcuts for Api::apply and Api::apply_status #649

clux opened this issue Oct 10, 2021 · 2 comments
Labels
api Api abstraction related apply patch or server-side apply blocked awaiting upstream work question Direction unclear; possibly a bug, possibly could be improved.

Comments

@clux
Copy link
Member

clux commented Oct 10, 2021

Saw that these were added by client-go and did some investigation so thought i'd add an issue here for it to at least discuss them.

What is it: see client-go interface for (say) Node:
https://github.com/kubernetes/client-go/blob/f6ce18ae578c8cca64d14ab9687824d9e1305a67/kubernetes/typed/core/v1/node.go#L53-L54

the implementation is just a dumb shortcut to calling the patch method, just coerces the patch into the type you asked for:
https://github.com/kubernetes/client-go/blob/f6ce18ae578c8cca64d14ab9687824d9e1305a67/kubernetes/typed/core/v1/node.go#L191-L243

Their ApplyOptions are simple:
https://github.com/kubernetes/apimachinery/blob/71bf7ba06cd490392e05cd6828ae24a69cb0ccaa/pkg/apis/meta/v1/generated.proto#L141-L160
basically the same as their PatchOptions..

the weirdest thing is they got alternate variants of their structs for apply builders(?). comparing the two:

they seem identical, so it must just be for the builders in applyconfigurations directory?

The shortcut does not deal with the awkward duplicate required metadata: { name: same_value_as_param } being part of the patch, and it technically allows mismatching patch types to the patch at the api level, so it doesn't provide any extra type safety for us i think? at least as long as patch options and apply options remain the same on the kubernetes side.

not sure how valuable this would be to us, but raising this here anyway so that can look back at an analysis, even if the result is a wontfix.

@clux clux added question Direction unclear; possibly a bug, possibly could be improved. api Api abstraction related labels Oct 10, 2021
@clux
Copy link
Member Author

clux commented Oct 11, 2021

Ah, one benefit on the client-go side is less arguments over its main Patch type which requires more. The new ones implicitly handle the .metadata.name (by forcing it in the builders, and presumably extracting from taht). The KEP is a little clearer in its goals: https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/2155-clientgo-apply . pretty reasonable api, and it gets some typing safety:

appsv1apply.Deployment("ns", "nginx-deployment").
  Spec(appsv1apply.DeploymentSpec().
    Replicas(0).
    Template(
      v1apply.PodTemplate().
        Spec(v1apply.PodSpec().
          Containers(
            v1apply.Container().
              Name("nginx").
              Image("nginx:1.14.2"),
            v1apply.Container().
              Name("sidecar").
          )
        )
      )
    )
  )

over us just dumping it into json! and attempt-serializing at runtime.

@clux clux added the apply patch or server-side apply label Oct 30, 2021
@clux
Copy link
Member Author

clux commented Oct 30, 2021

The KubeCon America talk; SIG API Machinery Deep Dive goes through why these structs (with full optionals) are needed (in the first half of that talk); some required fields get defaulted when serialized through apply (and thus we might overwrite something like replica numbers with int default 0 when we use Default::default on a openapi struct and pass it to apply).

This might be something we could resolve long term in codegen. But for now using json! is the easier way to do it in rust (to avoid accidentally passing on non-optional defaults with server side apply).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Api abstraction related apply patch or server-side apply blocked awaiting upstream work question Direction unclear; possibly a bug, possibly could be improved.
Projects
Status: Defining
Development

No branches or pull requests

1 participant