-
Notifications
You must be signed in to change notification settings - Fork 441
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
Prefix dashboard IDs with package names #320
Conversation
@@ -133,7 +133,7 @@ | |||
"title": "[Metrics Linux] Host Services Overview", | |||
"version": 1 | |||
}, | |||
"id": "system-c431f410-f9ac-11e9-90e8-1fb18e796788", |
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.
@fearful-symmetry does this change look right to you?
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.
CI build is green. LGTM!
@@ -8,7 +8,7 @@ | |||
"highlightAll": true, | |||
"query": { | |||
"language": "kuery", | |||
"query": "" | |||
"query": "(data_stream.dataset:windows.application OR data_stream.dataset:windows.forwarded OR data_stream.dataset:windows.powershell OR data_stream.dataset:windows.powershell_operational OR data_stream.dataset:windows.security OR data_stream.dataset:windows.sysmon_operational OR data_stream.dataset:windows.system)" |
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.
Hey @leehinman, @narph, this PR has been merged but I just wanted to double check that this change to the windows
package is okay.
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 think we can drop windows.application & windows.system, those will end up in the system package. This probably means I need to go through and fix up the others as well.
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.
Thanks for checking, @leehinman. Would you mind putting up a PR with the relevant changes (and a package version bump)? That'll let me move forward with elastic/package-storage#495 for now, knowing that we'll end up with another, similar PR after your changes go in.
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 guess a better question to ask is: given that this PR is merged and a new version of this package is about to be published to the snapshot
package registry (elastic/package-storage#495), is it safe to go ahead with that publish PR that contains the above query?
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.
Yep, safe to go ahead.
@@ -121,6 +121,9 @@ | |||
"migrationVersion": { | |||
"dashboard": "7.3.0" | |||
}, | |||
"namespaces": [ | |||
"default" | |||
], |
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.
Hey @leehinman, @narph, this PR has been merged but I just wanted to double check that this change to the windows
package is okay.
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.
LGTM
* Prefix dashboard IDs with package names * Fixing formatting * Bumping up affected packages' versions
What does this PR do?
This PR iterates over all integrations packages and checks if any of their dashboard assets have IDs that do not start with the package's name. If such assets are found, their IDs are fixed to start with the package name and any references to those IDs from other asset files are updated.
Script used to make this change: https://gist.github.com/ycombinator/ff4cd5cd4c1fca6f1ae82a496c02e190
Related issues