-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Add data.json files for vsphere module. WIP #4209
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
Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run. |
Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run. |
It would be nice if they would be auto generated with generate-json. If you have a local setup and the data tests, you could run |
@ruflin I'm not sure if it's right but I included TestData on the tests so now the data.json file is generated based on simulator data. |
@amandahla That actually sounds great. |
jenkins, test it |
@ruflin I made a Kibana Dashboard for this module with a few visualizations. Can I commit here or open another PR? Thanks! |
Nice. Lets open a new PR so we can merge this one sooner. |
Hi @amandahla, in master, I'm seeing some test failures. Maybe they are already fixed by this PR, if not could you please take a look. Let me know if you need any more info. From jenkins on linux:
From a windows vm:
|
I opened #4220 for the above issue. |
@amandahla Could you squash your commits into 1 commit and rebase on master? Also run |
jenkins, test it |
f := find.NewFinder(m.Client, true) | ||
if f == nil { | ||
return nil, errors.New("Finder undefined for vsphere.") |
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.
[golint] reported by reviewdog 🐶
error strings should not be capitalized or end with punctuation or a newline
@amandahla PR looks good to me. Could you rebase again on master and fix the golint issue above? |
jenkins, test it |
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.
There seems to be a merge problem. The fixes from https://github.com/elastic/beats/pull/4220/files are lost.
@@ -62,7 +62,7 @@ func New(base mb.BaseMetricSet) (mb.MetricSet, error) { | |||
func (m *MetricSet) Fetch() ([]common.MapStr, error) { | |||
f := find.NewFinder(m.Client, true) | |||
if f == nil { |
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.
Unfortunately find.NewFinder
is lacking godocs to document the function's contract. But based on the implementation, the function cannot return nil
so this check is unnecessary.
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.
@andrewkroh Sorry, maybe my rebase went wrong. I'll correct this and remove this check. In another PR, I'll see if it's possible to use the new package view instead of finder (like 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.
There still seem to be some rebase issues. Did you rebase on top of the most recent version of master? Things seem to be in the diff which shouldn't. Not sure what happened.
@@ -29,7 +29,7 @@ type MetricSet struct { | |||
} | |||
|
|||
func New(base mb.BaseMetricSet) (mb.MetricSet, error) { | |||
logp.Experimental("The vsphere host metricset is experimental") | |||
logp.Warn("EXPERIMENTAL: The vsphere host metricset is experimental") |
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 change should not be reverted.
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.
Sorry. I tried to make the same as here so I change this warning.
https://github.com/elastic/beats/pull/4220/files
@@ -30,7 +30,7 @@ type MetricSet struct { | |||
} | |||
|
|||
func New(base mb.BaseMetricSet) (mb.MetricSet, error) { | |||
logp.Experimental("The vsphere virtualmachine metricset is experimental") | |||
logp.Warn("EXPERIMENTAL: The vsphere virtualmachine metricset is experimental") |
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 change should not be reverted.
jenkins, test it |
@amandahla Thank you for your PR. It would be nice if you could a Kibana dashboard as well. Thanks! |
Related to #4028
Todo: