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.

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

Incorporate Viper #1827

Merged
merged 3 commits into from
Nov 17, 2022
Merged

Incorporate Viper #1827

merged 3 commits into from
Nov 17, 2022

Conversation

rexagod
Copy link
Member

@rexagod rexagod commented Sep 4, 2022

What this PR does / why we need it: Add --config flag to support supplying options from a config file, and hot-reload on any changes to it.

How does this change affect the cardinality of KSM: No change.

Which issue(s) this PR fixes: Fixes #1738

@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 Sep 4, 2022
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 4, 2022
@mrueg
Copy link
Member

mrueg commented Sep 4, 2022

/hold
I would suggest to resolve this as part of #1791

@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 Sep 4, 2022
@dgrisonnet
Copy link
Member

I agree but I would still prefer to have the efforts split into different PRs since it will be easier to review.

  1. Update CLI toolkit
  2. Add config-file option
  3. Implement config reloading

@rexagod rexagod marked this pull request as ready for review September 9, 2022 10:28
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 9, 2022
@rexagod
Copy link
Member Author

rexagod commented Sep 9, 2022

ACK @dgrisonnet, I'll break this PR up.

@rexagod rexagod mentioned this pull request Sep 14, 2022
@rexagod rexagod changed the title Add --opts-config-file flag Incorporate Viper Sep 15, 2022
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2022
pkg/app/server.go Outdated Show resolved Hide resolved
@rexagod
Copy link
Member Author

rexagod commented Nov 6, 2022

Ready for review.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2022
go.mod Outdated Show resolved Hide resolved
@rexagod rexagod requested review from mrueg and removed request for dgrisonnet November 16, 2022 13:45
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 16, 2022
@rexagod
Copy link
Member Author

rexagod commented Nov 16, 2022

Tidying up the commits.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2022
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2022
--config flag defines the path to the kube-state-metrics options config file.
Reload KSM on-the-fly when a change is detected in the configuration.

Meta changes (will squash)
Add end-to-end testing to verify hot-reloading for event-based changes
stemming from the config file. Also,
* sent in a doc fix that was missed
earlier:
https://github.com/kubernetes/kube-state-metrics/pull/1890/files#diff-380eca5a922c0ddbf67f04daefc6823e7ef0e197434d3a826d39c7063cdfa6d6R15,
* updated fsnotify and viper dependencies (v1.6.0 and v1.14.0
  respectively).

Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2022
@rexagod
Copy link
Member Author

rexagod commented Nov 16, 2022

Ready for review.

@mrueg
Copy link
Member

mrueg commented Nov 17, 2022

/hold cancel
/lgtm

Thanks for this contribution @rexagod !

@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 Nov 17, 2022
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 17, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrueg, rexagod

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 Nov 17, 2022
@k8s-ci-robot k8s-ci-robot merged commit 34b4660 into kubernetes:master Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to configure --metric-allowlist and --metric-denylist after startup
5 participants