Skip to content
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

Set organization home dashboard #1301

Merged
merged 5 commits into from
Nov 21, 2023
Merged

Conversation

jaehnri
Copy link
Contributor

@jaehnri jaehnri commented Nov 7, 2023

Closes #896

jaehnri and others added 3 commits November 7, 2023 16:06
Signed-off-by: João Henri <joao.rocha@suse.com>
Signed-off-by: João Henri <joao.rocha@suse.com>
Signed-off-by: João Henri <joao.rocha@suse.com>

wip: Use PUT endpoint
@jaehnri jaehnri marked this pull request as ready for review November 7, 2023 22:48
return err
}

p, err := grafanaClient.OrgPreferences()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a small bug in the grafana-api-go-client that makes us unable to use the PATCH endpoint, having to GET first and then PUT. I solved it in this PR, so I'm waiting for a new release to change this.

Copy link
Collaborator

@weisdd weisdd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaehnri thanks for the PR!

Though, if I understand your code correctly, you're suggesting to update the org home dashboard upon reconciling any of the dashboards. That means, for 100 dashboards, the org settings will be updated 100 times.
Plus, Grafana API refuses to update the setting when you pass an uid that does not exist yet. Say, we have 100 dashboards and the operator just started. Then it can potentially run into two issues:

  • If we look at the queue, and the target dashboard is on the 100th place there, the reconciliations for the first 99 dashboards will fail;
  • If you specify a non-existent uid, reconciliation will never succeed for any of the dashboards.

Unfortunately, this setting is tricky, we need to come up with something more nuanced.

@jaehnri
Copy link
Contributor Author

jaehnri commented Nov 13, 2023

Hi, @weisdd! Thanks for the review.
We will only update Grafana Preferences whenever the check below is true:

grafana.Spec.Preferences != nil && uid == grafana.Spec.Preferences.HomeDashboardUID

That way, if a nonexistent dashboard is chosen, the condition above never holds true and simply no home dashboard will ever be set up. Also, we only send the PATCH after the dashboard creation, so we will never send a request with a UID that Grafana does not know (the uid variable is taken from the dashboardModel, used in the dashboard reconciliation). So, if a dashboard is not the home one, it simply skips this step, but it won't fail its reconciliation.

Also, it only makes the API call for the home dashboard after the check above, so there's no chance we will update the settings 100 times.

@weisdd
Copy link
Collaborator

weisdd commented Nov 13, 2023

@jaehnri Alright, it should work then. We'll look at the PR deeper in the coming days. Thanks for the clarification!

@NissesSenap
Copy link
Collaborator

For me, it feels a bit strange to have this config in the grafana CRD but have the Dashboard reconciler doing the work.
I can see how this will be a bit strange to debug in the future.

It especially gets strange if we want to start setting org timezone or grafana extend the api/org/preferences with something more useful that we might want to support.
At the same time, I understand that making this logic in the dashboard reconcile makes verifying that the UID exist much easier.
I don't have any good ideas for a better solution. At the same time, we should be practical. There is none that has pushed for these features earlier, so why would that change now?

So in the spirit of being practical, LGTM.

Copy link
Collaborator

@weisdd weisdd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NissesSenap I'd agree, it's a good enough solution for now. If we ever come up with a better idea, we can reimplement the solution then.

As a side note, I've added a tiny fix - updated bundle manifest (make bundle).

@weisdd
Copy link
Collaborator

weisdd commented Nov 21, 2023

By the way, when a dashboard is deleted, the setting gets reset by Grafana itself, which nice :)

@jaehnri
Copy link
Contributor Author

jaehnri commented Nov 21, 2023

Thanks for the review! I agree the solution isn't as elegant as one would hope but it does work.
Still waiting for the grafana-api-golang-client fix, but I can open another PR whenever that gets released.

@NissesSenap NissesSenap merged commit b48786a into grafana:master Nov 21, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set organization homeDashboardUID through the grafana operator
3 participants