-
Notifications
You must be signed in to change notification settings - Fork 65
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
Add grafana links to hubs list #817
Conversation
# Pull support chart information to populate fields (if it exists) | ||
support = cluster.get("support", {}).get("config", {}) | ||
grafana_url = support.get("grafana", {}).get("ingress", {}).get("hosts", "") | ||
if isinstance(grafana_url, list): |
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.
Are we really expecting a list of hosts urls? And if that is the case, why do we choose just the first one?
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.
Yeah, the grafana
chart expects ingress.hosts
to be a list:
I think if we are following the same policy for grafana that we do for hubs, that we don't support multiple domains, then there should only be one entry here. Maybe that is something we can enforce in the schema?
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.
yeah I was just taking the first item because it always seemed to be a list in the config. Is this something that should block this PR? I think building the docs will fail if this condition is ever not met so we'd catch it then
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.
Should we capture @sgibson91's suggestion in a new issue?
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.
+1 from me, though I think it is probably not a huge priority
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 am going to merge this one in because I would personally find it very useful to be able to quickly reference the grafana URLs for hubs (and make it clear when we don't have a grafana set up for a hub...but happy to iterate on questions about the right configuration structure etc in follow-up issues! |
The grafana dashboards for our clusters are configured in the cluster-specific files in this repository. Since those exist in a structured space, this PR grabs the grafana configuration for each cluster and generates a grafana link per hub. That way we can more easily reference the grafana URL for a given hub, as this may be a common action and this can help reduce confusion.
you can preview how this looks here: https://2i2c-pilot-hubs--817.org.readthedocs.build/en/817/reference/hubs.html
This is a follow-up to #815.