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

⚠ Client Server side apply #2702

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

⚠ Client Server side apply #2702

wants to merge 1 commit into from

Conversation

troy0820
Copy link
Member

@troy0820 troy0820 commented Mar 7, 2024

This is an attempt at server side apply being applied to the client.Writer interface.

@troy0820
attempt at server side apply
d4062d6 
Signed-off-by: Troy Connor <[email protected]>
@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 Mar 7, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: troy0820
Once this PR has been reviewed and has the lgtm label, please assign alvaroaleman for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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 size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 7, 2024
@troy0820
Copy link
Member Author

troy0820 commented Mar 7, 2024

/test all

@k8s-ci-robot
Copy link
Contributor

@troy0820: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-controller-runtime-apidiff d4062d6 link false /test pull-controller-runtime-apidiff

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@troy0820
Copy link
Member Author

troy0820 commented Mar 7, 2024

This feels wrong, but may be right? Any feedback is definitely appreciated. I do know that the test failed on core object without the type meta because I didn't get the gvk of the client.Object before converting, however this isn't tested against a CRD.

Also, not sure if we want to allow applyconfigurations or just deal with the unstructured parts we are tryin to "apply".

Didn't want to try to resolve this myself as this may need discussion.

alvaroaleman
alvaroaleman reviewed Mar 8, 2024
View reviewed changes
pkg/client/client.go
@@ -347,6 +348,21 @@ func (c *client) Patch(ctx context.Context, obj Object, patch Patch, opts ...Pat
}
}

func (c *client) Apply(ctx context.Context, obj Object, fieldOwner string) error {
Copy link
Member

Choose a reason for hiding this comment

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

Pretty heavily opposed to do anything in here before kubernetes/kubernetes#118138 is done because:

  • The only type that can be used in here is unstructured.Unstructured
  • The converting as you do it down below doesn't fix the issue that the normal types end up putting zero values in fields
  • Which is the reason why applytypes exist, but you can not actually use applytypes here, because they do not fulfill client.Object
    /hold

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely hold, and no strong opinions on the feedback beyond understanding the gaps that exists. Waiting on this make sense and provided the support going forward, strategy around this seems crucial. Here for that conversation.

Copy link
Member

Choose a reason for hiding this comment

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

The whole topic boils down to "someone needs to do the work":

  1. The upstream issue I mentioned needs fixing, upstream signaled that they are ok with the idea but no one has shown up to actually put the work in
  2. This PR needs to be finished: [WIP] ✨ Generation of typed apply clients using upstream generator controller-tools#818
  3. A change like this in controller-runtime that adds an Apply method that only accepts apply types

@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 Mar 8, 2024
@Kavinjsir Kavinjsir mentioned this pull request Apr 1, 2024
Merged
@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 Jun 6, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active 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 rotten
  • Close this 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 Jul 6, 2024
@troy0820
Copy link
Member Author

I can close this PR but I wanted to track the work that's necessary and the conversation from this review. The apply types that are needed from controller-tools still has a WIP PR out, and the method must accept apply types.

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jul 31, 2024
@sbueringer
Copy link
Member

/lifecycle frozen

@k8s-ci-robot
Copy link
Contributor

@sbueringer: The lifecycle/frozen label cannot be applied to Pull Requests.

In response to this:

/lifecycle frozen

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-sigs/prow repository.

@hawkowl hawkowl mentioned this pull request Aug 5, 2024
Merged
@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 Oct 30, 2024
@troy0820
Copy link
Member Author

/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 Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alvaroaleman alvaroaleman alvaroaleman left review comments

@varshaprasad96 varshaprasad96 Awaiting requested review from varshaprasad96

@vincepri vincepri Awaiting requested review from vincepri

Assignees
No one assigned
Labels
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. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@troy0820 @k8s-ci-robot @k8s-triage-robot @sbueringer @alvaroaleman