Remove unused visualizations #3975
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
🌐 Coverage report
|
|
From @elastic/obs-cloudnative-monitoring side are you referring to traefik and nginx-ingress-controller ones. Seem good and you can remove those visulaisations. Although not sure 100% if we are the owners of those I approved for those the PR |
|
@elastic/security-external-integrations and @elastic/obs-service-integrations could I please get a review on this? |
|
💡 I think it would be nice to have a warning in the elastic-package tool for unused visualizations. |
|
++ on having a potential warning for viz without reference @elastic/ecosystem team. In theory, someone could build a package with just a visualisations inside but I'm not aware of any package that has this today. I really appreciate that @flash1293 opened this PR. But at the same time it would be nice if each time could take the diff out of this PR and open a PR for each package that needs it. This will make sure each team can check the package with the removed visualisations to be valid. |
|
It should be fairly simple to do this check - for each package, see whether the ids of visualizations/searches/lenses/maps show up in the |
|
To make sure this doesn't get stuck and caught up in conflicts, @andrewkroh @gizas @lalit-satapathy should this PR be merged as is and your team will publish the new versions of the packages or do you pick the changes and open your own PRs. One thought: Do we really need to increase the version of the packages. I think this is one of the changes where we actually don't need a new release yet of the packages but can wait until a next change happens. Changelog entry is of course still needed. |
In that case the next author needs to remember to adjust the version in the changelog, I don't think this is something that has been done before. Also this cleans up the library, so you could argue it's an improvement for the user (albeit a small one). |
I would prefer to merge this PR as is. I'll take care of promoting the new packages owned by security-external-integrations. |
|
@andrewkroh Also fine by me. Can you resolve conflicts and get it in? |
|
Resolved conflicts, @andrewkroh do you know who from @elastic/obs-service-integrations could take a look? |
|
@lalit-satapathy @tommyers-elastic @gizas Could you take a look? |
|
Did another round and all seem good! Double checked and there are no references for the above visualisations. Approved |
Changes look good for one package (postgreSQL) that I checked. Added @ManojS-shetty go through all the service integration packages once and confirm there is no issues. |
|
Hi @flash1293 , Just a quick question, Just observed in apache package as we removed the unused visualisation and i saw in the /img directory for a screenshot image for the dashboard contains the removed visualisation snap. Is that ok here? or we just need to have a dashboard screenshot image which does not show the removed visualisation on the screenshot image. |
|
@ManojS-shetty i guess the screenshots should be updated but it’s unrelated to this pr as the visualization wasn’t part of the dashboard even without my changes. |
|
/test |
|
/test |
What does this PR do?
I noticed there were a bunch of trailing visualizations not used on dashboards - I removed them. The owner teams should make sure this makes sense or whether these visualizations were relied on in some way other than adding them to a dashboard.