-
Notifications
You must be signed in to change notification settings - Fork 456
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
[System] Migrate dashboards to "by value" #5023
[System] Migrate dashboards to "by value" #5023
Conversation
…e Metrics dashboard
🌐 Coverage report
|
dynamic_fields: | ||
"@timestamp": "^[0-9]{4}(-[0-9]{2}){2}T[0-9]{2}(:[0-9]{2}){2}\\.[0-9]{3}" |
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.
As suggested in #4950, added this dynamic field to fix tests due to timestamps:
--- want
+++ got
@@ -1,7 +1,7 @@
{
"expected": [
{
- "@timestamp": "2022-05-21T21:54:44.000Z",
+ "@timestamp": "2023-05-21T21:54:44.000Z",
"ecs": {
"version": "8.0.0"
},
b147231
to
936d41c
Compare
"migrationVersion": { | ||
"visualization": "7.10.0" | ||
"visualization": "8.0.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.
Don't know why this namespaces field is removed.
These dashboards have been exported using elastic-package export dashboards
command.
packages/system/changelog.yml
Outdated
- version: "1.21.0-next" | ||
changes: | ||
- description: Migrate dashboards to be by value | ||
type: enhancement | ||
link: https://github.com/elastic/integrations/pull/5023 |
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.
Added just an entry in changelog with -next
suffix so it is not going to be published.
In this way, @elastic/elastic-agent-data-plane can decide when it needs to be released by updating the manifest and removing this suffix in another PR.
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
Pinging @elastic/security-external-integrations (Team:Security-External Integrations) |
I think some of the visualizations here were created by the SEI team, we will want them to review 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.
I tested that they all still load in 8.1.0. 👍 (I didn't have data populated for them though)
Apparently I wasn't testing 1.21.0-next. I used elastic-package install
successfully, but I still had 1.20.4 installed. So I can't say that I tested 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.
This time I can confirm that they all still load (😄 and that I was testing this code).
packages/system/changelog.yml
Outdated
@@ -1,4 +1,9 @@ | |||
# newer versions go on top | |||
- version: "1.21.0-next" |
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.
Do we normally tag versions here with -next
? Don't know if I've seen that on an integration before?
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.
If -next
is added to the version, it is not published. This can be used to allow other PRs to be added without releasing.
Here is a doc about it where it shows a possible use case: https://github.com/elastic/integrations/blob/main/docs/developer_workflow_design_build_test_integration.md#remember-to-bump-up-the-version
I set the version as this (without updating manifest.yml), so the package owner team can release this when they decide.
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.
As talked to @jlind23 and @pierrehilbert about this versioning, I'll update the changelog entry and the manifest to be 1.21.0
version. So once it is merged, it will be published.
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 great.
But I'm wondering: why was this done manually instead of using our automated tooling?
I didn't know about that tool, and as I needed to just migrate specific visualizations I did it manually following the process in the description of the PR. Taking advantage of the "Unlink from library" button and using |
Package system - 1.21.0 containing this change is available at https://epr.elastic.co/search?package=system |
What does this PR do?
Migrate the visualizations used in the dashboards that are by reference to be by value.
Just the visualizations that are used in one dashboard are migrated. Those visualizations that are shared between two or more dashboard are left as they are.
There were 13 dashboards using visualization by reference and now there are 12.
There is also a decrease in the number of visualizations since they are now in the dashboards by value. From 103 to 45 visualizations.
Process followed to migrate the dashboards:
elastic-package
. According to the manifest: 8.1.0 (link)elastic-package
command:elastic-package export dashboards
NOTE: not able to test the dashboards with data. This migration has been carried out using a local Elastic stack by means of
elastic-package
Checklist
changelog.yml
file.Related issues
Screenshots