-
Notifications
You must be signed in to change notification settings - Fork 305
Add another couple of constructors to Fix #279 #302
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
Welcome @DavidParks8! |
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. |
Signed the CLA |
Thanks for the PR! The diffs here make me nervous that we're introducing a regression (and I'm not 100% how perfect our coverage is here...) Any chance you can refactor the PR to minimize diffs? It looks like you included some cleanups along with new code. Any chance you can break them into two commits (or two PRs) so that I can review the cleanup and the new code separately? Thanks! |
When implementing this, I faced the issue of either deduplicating, or copying the new constructors for each isolated platform. I chose to deduplicate (either way, the changeset would be large unless this PR didn't include parity across platforms). Since the major deduplication happened for the mono platform, I went ahead and dereferenced the new code into what the existing constructor would look like if the functions were inlined, then did a diff with the constructor from before this change. This resulted in the following changes: https://www.diffchecker.com/h24E7Ct7 Is that sufficient to show the relative risk? |
|
||
CaCerts = config.SslCaCerts; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks to me like this code was dropped? (I could be wrong...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, is it in the constructor above? I would just move all of that down here....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would, but doing so won't compile because CaCerts only has an empty getter that needs to be assigned where it is declared, or directly within a constructor.
Ok, I'm more comfortable with this now, one note about moving where CaCerts and IsTLSVerify is initialized and I think we're good to merge. Thanks for the patience. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, DavidParks8 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 |
Thanks! |
…rnetes-client#302) * Add support for HttpClientFactory with KubernetesClientConfiguration * Add an example with http client factory * ensure sample uses the latest version of C#
This change deduplicates some of the code of the Kubernetes class, and adds the ability to use KubernetesClientConfiguration with the http client factory pattern.
A sample project was added to show how this pattern might be used.