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

Add ability to provide compressed JSON for large dashboards #767

Merged
merged 17 commits into from Jul 12, 2022

Conversation

meln5674
Copy link
Contributor

@meln5674 meln5674 commented May 28, 2022

Description

  • Added fields gzipJson and gzipConfigMapRef to GrafanaDashboardSpec
  • Added unit tests for decoding/decompression
  • Added docs explaining how to compress dashboard data
  • Added example compressed dashboards to verify functionality

Relevant issues/tickets

Fixes #726

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • This change requires a documentation update
  • I have added tests that prove my fix is effective or that my feature works
  • I have added a test case that will be used to verify my changes
  • Verified independently on a cluster by reviewer

Verification steps

Tested locally via the following

kind create cluster
make VERSION=testing manifests docker-build install deploy
kind load docker-image quay.io/grafana-operator/grafana-operator:vtesting
kubectl -n grafana-operator-system patch deploy/grafana-operator-controller-manager \
  --type=strategic --patch='{"spec":{"template":{"spec":{"containers":[{"name": "manager", "imagePullPolicy": "Never"}]}}}}'
kubectl -n grafana-operator-system create -f deploy/examples/Grafana.yaml
kubectl -n grafana-operator-system create -f deploy/examples/dashboards/
while ! kubectl -n grafana-operator-system get deploy grafana-deployment; do sleep 5; done
kubectl -n grafana-operator-system wait deploy grafana-deployment --for condition=Available=true --timeout=-1s
xdg-open http://localhost:3000/dashboards
kubectl -n grafana-operator-system port-forward svc/grafana-service 3000:3000

meln5674 added 2 commits May 28, 2022
* Added fields gzipJson and gzipConfigMapRef to GrafanaDashboardSpec
* Added unit tests for decoding/decompression
* Added docs explaining how to compress dashboard data
* Added example compressed dashboards to verify functionality
@meln5674 meln5674 force-pushed the feature/grafana-operator-726 branch from ab9c232 to a352f0a Compare May 28, 2022
@meln5674 meln5674 force-pushed the feature/grafana-operator-726 branch from a352f0a to 04bdf2b Compare May 28, 2022
@NissesSenap
Copy link
Member

NissesSenap commented May 29, 2022

@meln5674 thanks for the PR. The reason why the CI failes is due to that you are using go >=1.17.
You are not the first to commit this change but since we are always checking for dirt in git you get the error.
I have opend a PR to upgrade to 1.18 in the operator: #768 and when that gets merged you shoulden't have to think about this.

Feel free to wait for that to get merged or you can just remove the update that you did with:

--- a/api/integreatly/v1alpha1/zz_generated.deepcopy.go
+++ b/api/integreatly/v1alpha1/zz_generated.deepcopy.go
@@ -1,4 +1,3 @@
-//go:build !ignore_autogenerated
 // +build !ignore_autogenerated

About the markdown errors please have a look at them.
I haven't had time to review the code, but I hope me or some other maintainer will have time during this week.

@meln5674
Copy link
Contributor Author

meln5674 commented May 29, 2022

It turns out that's actually not because of the go version, but it seems to be getting auto-injected by vim every time I save. I can just remove it by hand in another editor.

As for the markdown linter, unless I'm misinterpreting the output, none of those are lines I touched:

~/git/src/github.com/meln5674/grafana-operator$ cat -n documentation/dashboards.md | grep -E '^\s*(27|187|193|196)'
    27    * _Note_: Folders with custom names are not managed by the operator, by purposeful design they won't be deleted when
   187  Default assignment of the dashboards to namespace-named folders is consider as a _managed folder_, this means that when
   193  specifies, this is considered as an _unmanaged folder_ and won't be deleted even if empty and will remain on the UI.
   196  _Note_ : Deletion of unmanaged folders requires manual intervention.

So I'm not entirely sure what the issue is.

@NissesSenap
Copy link
Member

NissesSenap commented May 29, 2022

@meln5674 it got something to do with your go version. I'm 99% sure because I have the same thing. Even if I don't touch that file when I run all the generate commands it will apply the exact same config.
If you want to verify you can downgrade golang to 1.16 and it will stop happening.

So for the last year I have removed this change manually every time ^^.

You can also take a look at my PR and I have added the change that is happening but since I have upgrade to 1.18 in the CI it now passes.

Even thgouh this file wast updated in this PR orignially I fix it here to pass the CI.
@NissesSenap
Copy link
Member

NissesSenap commented May 30, 2022

About the CI linter, I have no idea why it changed, my guess would be that the underlying dockerfile have been updated even though I used a tagged version.
I took the liberty of updating your PR with fixes for the error so the CI would pass.

Copy link
Member

@NissesSenap NissesSenap left a comment

Really nice job, tests, fixing docs and providing examples straight out of the box :).

Added a few comments for some extra CRD documentation.
Other then that I think it looks good.
@HubertStefanski @pb82 any thoughts?

api/integreatly/v1alpha1/grafanadashboard_types.go Outdated Show resolved Hide resolved
@meln5674
Copy link
Contributor Author

meln5674 commented Jun 20, 2022

Hey, just wanted to check in on this. I see there's been some merges from master into this, and the CI is complaining about something unrelated. Is this just a result of some CI changes you guys are in the middle of, or is there action required on my part?

@NissesSenap
Copy link
Member

NissesSenap commented Jun 21, 2022

@meln5674 if you have the time it would be great if you can fix the lint issues just to make the CI happy. Another option is just to ignore them.
Else I will try to get the time to update in a separate PR.

@meln5674
Copy link
Contributor Author

meln5674 commented Jun 22, 2022

I did a grep for the // nolint directive it seems to be complaining about, and its ubiquitous throughout the codebase, so something tells me the CI isn't doing what its supposed to.

@NissesSenap
Copy link
Member

NissesSenap commented Jun 22, 2022

I will have a look at it.

pb82
pb82 approved these changes Jul 12, 2022
Copy link
Contributor

@pb82 pb82 left a comment

Verified, thanks @meln5674

@pb82 pb82 merged commit 0b033e7 into grafana-operator:master Jul 12, 2022
8 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.

Support GZipped JSON Dashboards
4 participants