Main improved dashboard links #622
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for approving the workflows, will work to correct the tests shortly! |
|
Looks like the issue is with py37/38 support, will push some fixes shortly. |
|
Running locally against Python 3.8 passes now, screenshot is provided 🙂 |
JamesGibo
requested changes
Sep 11, 2023
There was a problem hiding this comment.
Thanks for the PR it looks really good and well documented.
I spotted one issue that might be type that I would like you to check.
If you had time, you could add a test for this class to the tests, but as this Class did not previously have a test, I am happy to merge without this.
Includes the ability to create direct links or a list of dashboard links, extra options like includeVars and targetBlank.
Removing this parameter is a breaking change... It didn't actually impact the generated link, other than acting as an alias to 'title' in some cases. If preferred, I can restore it to retain that alias behaviour, and perhaps add a warning log message?
padraic-padraic
force-pushed
the
main-improved-dashboard-links
branch
from
2df85b5
to
fb44dd6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this do?
Extends the implementation of DashboardLink to support creating both direct links and lists of dashboards.
Why is it a good idea?
This enables grafanalib users to fully employ the DashboardLink feature. The previous JSON model was no longer accurate for recent grafana versions, meaning dashboard links were not properly generated. Here we add all available fields, allowing users to create dashboard links correctly from grafanalib.
Context
Dashboard Links are a way to create links along the top of a dashboard, linking either to other sets of dashboards from the same organization or arbitrary external resources. See the documentation here: https://grafana.com/docs/grafana/latest/dashboards/build-dashboards/manage-dashboard-links/#manage-dashboard-links
Questions
I have removed a parameter from the old DashboardLink class,
dashboard; this could consistute a breaking change.This parameter was not actually consumed by Grafana, so was in effect only acting as an internal alias to the
titlefield. Thetitlewould only be shown if theDashboardLink.typeattribute is set tolink, something actually not possible before this PR. so I don't consider it to be a significant change. But, if desired, we can retain that alias-like behaviour to maintain API compatability.