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

✨ Generation of typed apply clients #536

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Jefftree
Copy link
Member

@Jefftree Jefftree commented Jan 19, 2021

Generate Apply clients for use with server side apply. For built in types, a hard coded mapping is done to point to the client-go/applyconfiguration types. For CRDs, we assume applyconfigurations live at <path-to-package>/ac directory which should be the case if imported CRD packages also use our generator.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 19, 2021
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 19, 2021
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 19, 2021
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 19, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 25, 2021
Copy link
Contributor

@DirectXMan12 DirectXMan12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(first pass on some high-level stuff)

pkg/applyconfigurations/gen.go Outdated Show resolved Hide resolved
pkg/applyconfigurations/traverse.go Outdated Show resolved Hide resolved
@Jefftree Jefftree changed the title [WIP] Generation of typed apply clients Generation of typed apply clients May 17, 2021
@Jefftree Jefftree marked this pull request as ready for review May 17, 2021 19:24
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 17, 2021
@Jefftree Jefftree changed the title Generation of typed apply clients ✨ Generation of typed apply clients May 17, 2021
Copy link

@jpbetz jpbetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I primarily reviewed the generated types. They look mostly correct, just some corner cases that were different than k/k or that might need some extra documentation.

pkg/genall/output.go Show resolved Hide resolved
cmd/controller-gen/main.go Show resolved Hide resolved
pkg/applyconfigurations/gen.go Outdated Show resolved Hide resolved
pkg/applyconfigurations/gen.go Show resolved Hide resolved
pkg/applyconfigurations/gen.go Outdated Show resolved Hide resolved
@alvaroaleman
Copy link
Member

/test all

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 20, 2021
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 19, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

@JoelSpeed
Copy link
Contributor

@EraYaN No, I actually deliberately omitted the Extract methods since I didn't see those being implemented as part of this PR. With the branch I linked in my previous comment, if you supply the openapi-schema to the generator then it will generate the Extract methods correctly, but it's not a requirement to have those as far as I understood.

I've been working on another branch which integrates directly the code into controller-tools and that again, doesn't generate the extract methods, but, if we could generate the openapi schema and pass it in, could do, perhaps a later extension?

@EraYaN
Copy link

EraYaN commented Feb 10, 2023

Generating the openapi.json doesn't seem to be trivial, so if at all possible it would be great if it ends up being all "batteries included". Operators like the prometheus-operator (EDIT: it was cert-manager) wrote some go tools to get the OpenAPI json dumped, but I feel that shouldn't be required for users to do. But it is also kind of a shame the openapi.json from an actual API server does not seem to work as a shortcut.

I like having the Extract methods since mostly we are doing something akin to this: https://github.com/zoetrope/kubebuilder-training/blob/main/codes/50_completed/controllers/markdownview_controller.go#L221-L253 Arguably we probably don't need those Extract calls and the Gets in the first place. But since it seems like it's one of the only complete examples using SSA extensively, it might be part of the story for quite some people.

@JoelSpeed
Copy link
Contributor

Generating the openapi.json doesn't seem to be trivial, so if at all possible it would be great if it ends up being all "batteries included".

You're right it's not the most straight forward thing to generate. By not supplying it, I think we are achieving what this PR set out to do, but, what is missing? The extract functions could possibly be generated separately, or, we can try and work out (maybe in a follow up PR), how to provide this and complete the generation

@camilamacedo86
Copy link
Member

@JoelSpeed do you agree within we moving forward with this one now?
We can improve it in follow ups. WDYT?

Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems fine for me.

/lgtm

Looking for @JoelSpeed and @vincepri and @alvaroaleman

Give their OK and remove the hold on this one.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 19, 2023
@vincepri
Copy link
Member

/approve cancel

@Jefftree Are you able to add tests and documentation before merging?

@alvaroaleman
Copy link
Member

@JoelSpeed mentioned they might be able to finish their branch after kubecon, which is likely the preferred approach as it avoids a lot of duplication

@JoelSpeed
Copy link
Contributor

I've been working on #818 today which I think is a good start on where we want to go. I would not recommend reviewing commit wise, it needs a lot of squashing, but, lets start working out what we want to see with this new integration. I'll leave a review myself with various points of UX I've thought of while working through

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 19, 2024
@EraYaN
Copy link

EraYaN commented Jan 19, 2024

This is still relevant
/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 19, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 18, 2024
@joelanford
Copy link
Member

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 18, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 17, 2024
@EraYaN
Copy link

EraYaN commented Jul 17, 2024

This is still relevant
/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.