-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fix BuilderInterface and BuildStoresFunc to allow using KSM as a library #1610
Fix BuilderInterface and BuildStoresFunc to allow using KSM as a library #1610
Conversation
bf3b1af
to
73d15e3
Compare
Thanks for this PR @ahmed-mez. We did not take into account that KSM might be used as a library as well. /lgtm |
Thanks @fpetkovski for approving! Happy to open another PR to backport the fix to v2.1 once this gets merged. |
Sure, that would be great! |
@fpetkovski what's the next step to get this PR approved / merged? |
/lgtm Are there any test cases etc. we could include to ensure that we don't break compatibility for external consumers? |
/lgtm |
/lgtm cancel |
Thanks @dgrisonnet - Comments addressed! |
f7c9c5d
to
4e7f6c3
Compare
limitations under the License. | ||
*/ | ||
|
||
package builder_test |
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.
should be builder
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.
this is actually a best practice to make sure we don't access private methods and functions in the builder
package.
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.
👍 It makes sense for your particular case since you want to test what is exported by kube-state-metrics. I am too used to white-box testing where the unit tests are accessing everything 😅.
4e7f6c3
to
698aee0
Compare
Looks good from my side thank you @ahmed-mez for taking the time to look into this. /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahmed-mez, clamoriniere, dgrisonnet, fpetkovski, mrueg 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 |
Unholding since we have 3 approvals. /unhold |
Thank you all for the reviews! Here is a PR to backport to v2.1 #1617 |
@ahmed-mez could you also create a backport to release-2.2? |
@dgrisonnet sure thing! #1618 |
[release-2.2] Backport #1610
What this PR does / why we need it:
This PR makes ksm usable again as a library after the breaking changes introduced by #1499
BuildStoresFunc
returns a generic[]cache.Store
to allow using custom storesbuilder.BuildStores()
which returns the stores as an alternative tobuilder.Build()
How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)
Does not change cardinality
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #1605