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.

Sign up for GitHub

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

Jump to bottom

Add support for loading dashboards in orgs #173

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add support for loading dashboards in orgs #173

wants to merge 5 commits into from

Conversation

hervenicol
Copy link
Contributor

@hervenicol hervenicol commented Nov 28, 2024 •
edited
Loading

What this PR does / why we need it

Towards giantswarm/roadmap#3696

Support loading dashboards in organizations.

Extract from README telling about this feature:

It will look for kubernetes ConfigMaps and use them as dashboards if they meet these criteria:

  • a label app.giantswarm.io/kind: "dashboard"
  • an annotation or label giantswarm.io/organization set to the organization the dasboard should be loaded in.

Current limitations:

  • no support for folders
  • each dashboard belongs to one and only one organization

Checklist

  • Update changelog in CHANGELOG.md.
  • Follow deployment test procedure in the tests/manual_e2e directory and have a working branch.

@hervenicol hervenicol self-assigned this Nov 28, 2024
@hervenicol hervenicol force-pushed the load-dashboards branch 19 times, most recently from beae65d to 9dc12d5 Compare December 5, 2024 11:13
@hervenicol hervenicol force-pushed the load-dashboards branch 4 times, most recently from b3a2637 to 3ba75a3 Compare December 5, 2024 17:10
@hervenicol hervenicol mentioned this pull request Dec 6, 2024
Closed
2 tasks
@hervenicol hervenicol force-pushed the load-dashboards branch 4 times, most recently from 809a0c1 to 61ce280 Compare December 13, 2024 13:07
@hervenicol hervenicol force-pushed the load-dashboards branch 4 times, most recently from a0bc613 to b7bfbdc Compare December 19, 2024 17:10
@hervenicol hervenicol marked this pull request as ready for review December 19, 2024 17:12
@hervenicol hervenicol requested a review from a team as a code owner December 19, 2024 17:12
TheoBrigitte
TheoBrigitte reviewed Dec 19, 2024
View reviewed changes
Copy link
Member

@TheoBrigitte TheoBrigitte left a comment

Choose a reason for hiding this comment

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

Nice job, sorry for all the remarks 🙈

internal/controller/dashboard_controller.go Outdated Show resolved Hide resolved
internal/controller/dashboard_controller.go Outdated Show resolved Hide resolved
internal/controller/dashboard_controller.go Outdated Show resolved Hide resolved
internal/controller/dashboard_controller.go Outdated Show resolved Hide resolved
internal/controller/dashboard_controller.go Outdated Show resolved Hide resolved
internal/controller/dashboard_controller.go Outdated Show resolved Hide resolved
internal/controller/dashboard_controller.go Outdated Show resolved Hide resolved
internal/controller/dashboard_controller.go Outdated Show resolved Hide resolved
internal/controller/dashboard_controller.go Outdated Show resolved Hide resolved
internal/controller/dashboard_controller.go Outdated Show resolved Hide resolved
@hervenicol hervenicol mentioned this pull request Dec 20, 2024
Merged
2 tasks
@hervenicol
Copy link
Contributor Author

Nice job, sorry for all the remarks 🙈

Thanks!
I did a first round of fixes, and I added a " 👍 " to each remark I fixed.
Feel free to check them and for each remark either "Resolve" or add new comments if you consider my fixes were not good enough @TheoBrigitte 🙇

Then I'll keep going on with the remaining remarks.

@TheoBrigitte
Copy link
Member

Nice job, sorry for all the remarks 🙈

Thanks! I did a first round of fixes, and I added a " 👍 " to each remark I fixed. Feel free to check them and for each remark either "Resolve" or add new comments if you consider my fixes were not good enough @TheoBrigitte 🙇

Then I'll keep going on with the remaining remarks.

Did a round of review on the reviews, closed a couple and commented where needed

@hervenicol hervenicol force-pushed the load-dashboards branch 2 times, most recently from 000d49e to 3bf3865 Compare December 20, 2024 16:05
@hervenicol
Copy link
Contributor Author

Added dashboards deletion and a bit of rework:

  • change org only once per configmap (instead as for each dashboard in the CM)
  • in case of error for a dashboard, continue reconciling next dashboards for this configmap

QuentinBisson
QuentinBisson reviewed Dec 22, 2024
View reviewed changes
README.md
@@ -7,6 +7,18 @@ This operator is in charge of handling the setup and configuration of the Giant
It reconciles `cluster.cluster.x-k8s.io` objects and makes sure each `Cluster` is provided with:
- TODO(atlas) update this section

## Features

### Grafan dashboards provisioning
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### Grafan dashboards provisioning
### Grafana dashboards provisioning

QuentinBisson
QuentinBisson reviewed Dec 22, 2024
View reviewed changes
README.md

It will look for kubernetes `ConfigMaps` and use them as dashboards if they meet these criteria:
- a label `app.giantswarm.io/kind: "dashboard"`
- an annotation or label `giantswarm.io/organization` set to the organization the dasboard should be loaded in.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we use this label for giant swarm organizations so we probably should not use this but smth like observability.giantswarm.io/organization instead. WDYT?

QuentinBisson
QuentinBisson reviewed Dec 22, 2024
View reviewed changes
internal/controller/dashboard_controller.go
Cert: conf.Environment.GrafanaTLSCertFile,
Key: conf.Environment.GrafanaTLSKeyFile,
}
grafanaAPI, err := grafanaclient.GenerateGrafanaClient(conf.GrafanaURL, grafanaAdminCredentials, grafanaTLSConfig)
Copy link
Contributor

Choose a reason for hiding this comment

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

We have the same code here

observability-operator/internal/controller/grafanaorganization_controller.go

Line 44 in 69c4b70

// Get grafana admin-password and admin-user
. Maybe we should create a function or create the client in main?

QuentinBisson
QuentinBisson reviewed Dec 22, 2024
View reviewed changes
internal/controller/dashboard_controller.go
}

func getDashboardUID(dashboard map[string]interface{}) (string, error) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick but I don't think we need those blank lines :)

QuentinBisson
QuentinBisson reviewed Dec 22, 2024
View reviewed changes
internal/controller/dashboard_controller.go
}

// Create or update dashboard
_, err = r.GrafanaAPI.Dashboards.PostDashboard(&models.SaveDashboardCommand{
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we move that to the grafana pkg and not the controller?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheoBrigitte TheoBrigitte TheoBrigitte left review comments

@QuentinBisson QuentinBisson QuentinBisson left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@hervenicol hervenicol

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hervenicol @TheoBrigitte @QuentinBisson