Skip to content

✨ Generation of typed apply clients using upstream generator #818

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

Merged

Conversation

JoelSpeed
Copy link
Contributor

@JoelSpeed JoelSpeed commented May 19, 2023

This integrates the upstream applyconfig generator into controller-tools to allow generation of ApplyConfig style types from custom API types.

The aim here is to provide the pointer style structs that could then be passed to an Apply style method for using server side apply.

Setting this up as a WIP PR for now, current TODO list:

  • Work out if there's any extra stuff here that's not needed
  • Work out how to test this without the hack adding cronjob testdata to the go.mod
  • Do we need OpenAPI schema/do we need the extract functions?
    • Future work, after this PR has merged
  • General review of code and cleanup - this was quite hacky when I started the branch
  • How would a user want this integrated into their project?
    • For now they can add the correct markers and it will generate as a subfolder of their API

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 19, 2023
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 19, 2023
@JoelSpeed JoelSpeed force-pushed the applyconfig-gen-integration branch 2 times, most recently from 18594e4 to 74642c2 Compare May 19, 2023 09:38
enableTypeMarker = markers.Must(markers.MakeDefinition("kubebuilder:ac:generate", markers.DescribesType, false))
)

var importMapping = map[string]string{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we allow additional import mappings, I know the upstream applyconfiguration gen does

Copy link
Member

Choose a reason for hiding this comment

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

I think that makes sense, yeah (But we can do that in a follow-up)

@JoelSpeed
Copy link
Contributor Author

/test all

Need to work out how to test this, but want to see what the existing tests do with this so I can fix any failures introduced

@JoelSpeed
Copy link
Contributor Author

/test all

@JoelSpeed JoelSpeed marked this pull request as ready for review May 19, 2023 15:46
filesInMaster := make(map[string][]byte)
masterFileNames := sets.New[string]()
cronJobFS := os.DirFS(".")
masterPath := "applyconfiguration-master"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I can't change the directory to which we are generating the assets currently, this is what I came up with, create a copy that we keep checked in and then diff against that, if there are genuine changes we can update both copies.

Will look at gengo to see if there's a backwards compatible way to allow it to output to different places like OutputRule does for these generators

@JoelSpeed
Copy link
Contributor Author

/assign @alvaroaleman

@EraYaN
Copy link

EraYaN commented May 22, 2023

As an option I would love to have the Extract functions, we use them extensively in our operator (for better or for worse).

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 22, 2023
@sbueringer
Copy link
Member

@JoelSpeed Can we get rid of the vendor folder? It's probably on my side but the performance of the file tab is pretty bad with 1700 files.

@alvaroaleman
Copy link
Member

@JoelSpeed Can we get rid of the vendor folder? It's probably on my side but the performance of the file tab is pretty bad with 1700 files.

Second this, I can literally not review this through the GitHub UI

@JoelSpeed
Copy link
Contributor Author

@JoelSpeed Can we get rid of the vendor folder? It's probably on my side but the performance of the file tab is pretty bad with 1700 files.

Second this, I can literally not review this through the GitHub UI

Sorry about that, must have committed during my experimenting, dropped it now, commit history in need of a good clean though

Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

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

For an internal project, I added the +kubebuilder:ac:generate=true comment both on the package and on my type, running controller-gen with apply paths=./pkg/api/v1/ succeeds but doesn't generate anything - any idea what I am am doing wrong?

enableTypeMarker = markers.Must(markers.MakeDefinition("kubebuilder:ac:generate", markers.DescribesType, false))
)

var importMapping = map[string]string{
Copy link
Member

Choose a reason for hiding this comment

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

I think that makes sense, yeah (But we can do that in a follow-up)

Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

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

For an internal project, I added the +kubebuilder:ac:generate=true comment both on the package and on my type, running controller-gen with apply paths=./pkg/api/v1/ succeeds but doesn't generate anything - any idea what I am am doing wrong?

@sbueringer
Copy link
Member

sbueringer commented May 30, 2023

For an internal project, I added the +kubebuilder:ac:generate=true comment both on the package and on my type, running controller-gen with apply paths=./pkg/api/v1/ succeeds but doesn't generate anything - any idea what I am am doing wrong?

Potentially could be: #818 (comment)

@JoelSpeed JoelSpeed force-pushed the applyconfig-gen-integration branch from c599c42 to 804b842 Compare March 26, 2025 13:47
@sbueringer
Copy link
Member

Thx!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 26, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 51056a1fd6f10be8a831975e810ceb82afa1b23d

Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

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

Looks good, thank you so much for your work on this!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, JoelSpeed

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

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. 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.