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.

Sign up for GitHub

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

Jump to bottom

Generate Gateway API ApplyConfiguration and update Clientsets to support ServerSideApply #2537

Merged
merged 8 commits into from
Apr 3, 2024
Merged

Generate Gateway API ApplyConfiguration and update Clientsets to support ServerSideApply #2537

merged 8 commits into from
Apr 3, 2024

Conversation

dprotaso
Copy link
Contributor

@dprotaso dprotaso commented Oct 27, 2023 •
edited
Loading

What type of PR is this?
/kind feature

What this PR does / why we need it:
Server-Side Apply is a Kubernetes feature that allows multiple controllers to manage the same single resource.

Which issue(s) this PR fixes:
N/A

Does this PR introduce a user-facing change?:

Gateway API clientsets now support Server-Side Apply

Additional Notes:
To simplify review I suggest folks go through each commit

Fixes: #1639

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 27, 2023
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 27, 2023
@robscott
Copy link
Member

Thanks @dprotaso! While this looks reasonable, it's a large diff and doesn't seem like it's strictly necessary for v1.0.0. Since we've already passed RC2, I think we should wait for the next patch or minor release to include this.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 27, 2023
@dprotaso dprotaso mentioned this pull request Nov 30, 2023
Closed
@therealmitchconnors
Copy link

Thank you, @dprotaso, I am really looking forward to using this!

It may be helpful to add Extract functions, similar to https://github.com/kubernetes/client-go/blob/master/applyconfigurations/core/v1/configmap.go#L62.

@dprotaso
Copy link
Contributor Author

dprotaso commented Dec 7, 2023

It's there at the top-level - eg. https://github.com/kubernetes-sigs/gateway-api/pull/2537/files#diff-d518444c0b279f513a665c7bfc148619760c1e8fb8824dbde9896dbd03eb454bR61

Do you see it missing for a type?

@robscott
Copy link
Member

robscott commented Jan 5, 2024

Hey @dprotaso, sorry I'd lost track of this one! Do you mind adding something to the Implementer's Guide showing how implementations could make use of this?

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 5, 2024
@dprotaso
Copy link
Contributor Author

@robscott take a look - I've updated the implementors guide.

@dprotaso
Copy link
Contributor Author

rebased

dprotaso
dprotaso commented Feb 23, 2024
View reviewed changes
cmd/modelschema/main.go

// From https://github.com/kubernetes/kubernetes/blob/0712728ee9caab4d04ec0a751855aca8c891b690/pkg/generated/openapi/cmd/models-schema/main.go

package main
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to engage with the SSA folks/wg-expression to figure out where this should land - probably the codegen for SSA would make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a slack thread - https://kubernetes.slack.com/archives/C0123CNN8F3/p1708650369536409

robscott
robscott reviewed Feb 23, 2024
View reviewed changes
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks @dprotaso!

site-src/guides/implementers.md Outdated Show resolved Hide resolved
site-src/guides/implementers.md Outdated Show resolved Hide resolved
site-src/guides/implementers.md Outdated Show resolved Hide resolved
@dprotaso
Copy link
Contributor Author

dprotaso commented Mar 7, 2024

@robscott anything else for this PR?

@shaneutt shaneutt requested a review from robscott March 12, 2024 16:31
@dprotaso
Copy link
Contributor Author

dprotaso commented Apr 2, 2024

@robscott following up again

@dprotaso dprotaso mentioned this pull request Apr 2, 2024
Open
@robscott
Copy link
Member

robscott commented Apr 2, 2024

Thanks @dprotaso!

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, robscott

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 2, 2024
@dprotaso
Copy link
Contributor Author

dprotaso commented Apr 2, 2024

Let me rebase and run codegen again

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 2, 2024
@dprotaso
Copy link
Contributor Author

dprotaso commented Apr 2, 2024

@robscott rebased needs another LGTM

@robscott
Copy link
Member

robscott commented Apr 3, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 3, 2024
@k8s-ci-robot k8s-ci-robot merged commit fa99a78 into kubernetes-sigs:main Apr 3, 2024
8 checks passed
@therealmitchconnors
Copy link

Thank you! I'm really looking forward to using this!

@dprotaso dprotaso deleted the server-side-apply branch April 4, 2024 17:01
@dprotaso
Copy link
Contributor Author

dprotaso commented Apr 4, 2024

@therealmitchconnors are you able to try it out from HEAD?

I know with some K8s resources the OpenAPI schema didn't have the correct x-kubernetes-list-map-keys so SSA didn't work as expected (eg. PodSpec.Containers).

I haven't done an audit of the Gateway API to ensure everything is annotated properly - I'll make an issue for that.

@dprotaso dprotaso mentioned this pull request Apr 4, 2024
Open
@therealmitchconnors
Copy link

I'm on vacation for two weeks, but I might find time to kick these tires...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shaneutt shaneutt Awaiting requested review from shaneutt

@robscott robscott Awaiting requested review from robscott

Assignees

@robscott robscott

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. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The client is missing Apply() functions, making use alongside other client-go packages difficult
4 participants
@dprotaso @robscott @therealmitchconnors @k8s-ci-robot