Skip to content

⚠️ Deprecate client.Options.WarningHandler #2896

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

Conversation

dlipovetsky
Copy link
Contributor

As a follow-up to #2887, and the conversation in #2887 (comment), this deprecates client.Options.WarningHandler.

Instead of using client.Options.WarningHandler users define config.WarningHandler. This change

  1. Reduces the number of "sources" of warning handler configuration from two to one, and thereby reduces confusion.
  2. Allows users to define custom warning handlers, and thereby increases flexibility.

Deprecation and future removal does not change the default behavior, which is to de-duplicate and surface warnings.

- Deprecation and future removal does not change the default behavior,
  which is to de-duplicate and surface warnings.
- Uses config.WarningHandler to override default behavior. Describes
  this in the client.New godoc, adds an example test to demonstrate it,
  and a unit test to verify it.
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dlipovetsky
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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 31, 2024
@dlipovetsky
Copy link
Contributor Author

/cc @sbueringer

@k8s-ci-robot k8s-ci-robot requested a review from sbueringer July 31, 2024 17:32
@sbueringer
Copy link
Member

@dlipovetsky Talked to Vince & Alvaro. We would be also fine with directly removing the field. I think that's okay as folks can just easily pass in our implementation via rest.Config

@dlipovetsky
Copy link
Contributor Author

@dlipovetsky Talked to Vince & Alvaro. We would be also fine with directly removing the field. I think that's okay as folks can just easily pass in our implementation via rest.Config

Thanks! To keep things simple, I'll open a new PR and reference this one. I think that's ok, since this PR hasn't had implementation reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants