-
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
builder/main: allow collectors not enabled by default #793
builder/main: allow collectors not enabled by default #793
Conversation
internal/store/builder.go
Outdated
|
||
func availableCollectors() []string { | ||
c := []string{} | ||
for name, _ := range availableStores { |
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 a map already, so we can just check on existance of the collector key in this map directly in collectorExists
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.
Fixed
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.
one more than lgtm
main.go
Outdated
@@ -86,7 +86,9 @@ func main() { | |||
storeBuilder.WithEnabledResources(options.DefaultCollectors.AsSlice()) |
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.
even if it should be impossible, we should check the error here as well
main.go
Outdated
} else { | ||
klog.Infof("Using collectors %s", opts.Collectors.String()) | ||
storeBuilder.WithEnabledResources(opts.Collectors.AsSlice()) | ||
err = storeBuilder.WithEnabledResources(opts.Collectors.AsSlice()) |
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.
Can you remove the code duplication in L86 and L89? We could assign a variable to hold either options.DefaultCollectors
or ops.Collectors
and then call storeBuilder.WithEnabledResources
. once.
Thanks for the fix! /lgtm |
main.go
Outdated
err = storeBuilder.WithEnabledResources(opts.Collectors.AsSlice()) | ||
} | ||
if err != nil { | ||
klog.Fatalf("Error: %s", err) |
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.
Something more descriptive here? and you should be using %v instead of %s.
/hold @tariq1890 comment on the error message is valid |
Thanks @milesbxf. Once the review comments are addressed, please squash the commit history :). |
e18cf74
to
21c8293
Compare
@tariq1890 @brancz done - thanks for looking at it! |
go.sum
Outdated
@@ -10,13 +10,10 @@ github.com/dgrijalva/jwt-go v0.0.0-20160705203006-01aeca54ebda h1:NyywMz59neOoVR | |||
github.com/dgrijalva/jwt-go v0.0.0-20160705203006-01aeca54ebda/go.mod h1:E3ru+11k8xSBh+hMPgOLZmtrrCbhqsmaPHjLKYnJCaQ= | |||
github.com/evanphx/json-patch v4.1.0+incompatible h1:K1MDoo4AZ4wU0GIU/fPmtZg7VpzLjCxu+UwBD1FvwOc= | |||
github.com/evanphx/json-patch v4.1.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk= | |||
github.com/fsnotify/fsnotify v1.4.7 h1:IXs+QLmnXW2CcXuY+8Mzv/fWEsPGWxqefPtCP5CnV9I= |
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.
Not relevant to this PR. Looks like the current go modules CI check is not working as expected. That might need fixing. FYI @brancz .
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.
agreed, let's handle separately
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.
Whoops spoke too soon. The CI checks are working as expected :). Can you run go mod tidy
and go mod vendor
@milesbxf ?
/hold cancel |
21c8293
to
14fd017
Compare
This fixes an issue where it was impossible to specify a collector that was available but not selected by default. Instead of checking whether chosen collectors are valid at flag parse time, this moves the check into the builder, where we can reference it against the availableStores in the builder. As a bonus, the error message also prints out a list of available collectors: ``` kube-state-metrics --collectors non-existent-collector I0618 15:23:34.517532 50719 main.go:88] Using collectors non-existent-collector F0618 15:23:34.519132 50719 main.go:90] Error: collector non-existent-collector does not exist. Available collectors: persistentvolumeclaims,configmaps,limitranges,nodes,namespaces,persistentvolumes,pods,replicasets,services,cronjobs,deployments,ingresses,horizontalpodautoscalers,jobs,poddisruptionbudgets,secrets,certificatesigningrequests,daemonsets,endpoints,storageclasses,replicationcontrollers,resourcequotas,statefulsets ```
14fd017
to
e1b45de
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brancz, milesbxf 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 |
What this PR does / why we need it:
This fixes an issue where it was impossible to specify a collector that
was available but not selected by default.
Instead of checking whether chosen collectors are valid at flag parse
time, this moves the check into the builder, where we can reference it
against the availableStores in the builder. As a bonus, the error
message also prints out a list of available collectors:
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 #