-
Notifications
You must be signed in to change notification settings - Fork 397
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
feat: add ability to provide JSONnet project libraries to runtime inside operator #1212
feat: add ability to provide JSONnet project libraries to runtime inside operator #1212
Conversation
# Conflicts: # api/v1beta1/grafanadashboard_types.go # config/crd/bases/grafana.integreatly.org_grafanadashboards.yaml # config/grafana.integreatly.org_grafanadashboards.yaml # deploy/helm/grafana-operator/crds/grafana.integreatly.org_grafanadashboards.yaml # deploy/kustomize/base/crds.yaml # docs/docs/api.md # go.sum
Thanks for the PR! I intend to take a look at this later on today! |
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 will also try to take a look later. I added a few small comments for starters.
cpu: 200m | ||
memory: 100Mi | ||
cpu: 1 | ||
memory: 1024Mi | ||
requests: | ||
cpu: 100m | ||
memory: 20Mi | ||
memory: 100Mi |
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 agree that the resources are way too much, but let's change that in a separate PR for traceability.
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, totally agree. This trick was needed for testing the generation of our dashboard. It seems that new version of grafonnet is more resources giddy than previous
docs/docs/dashboards.md
Outdated
Because of the flexibility, simplicity and extendability of JSONnet it's one of the best choices fitting to the currently modern approach "something as a code", dashboards as a code in our case. | ||
In this picture of the world, Grafana operator acts as a bridge between the DaC and Grafana resource that helps developers to absorb the logic of how dashboards deliver to target and helps to separate the responsibility between SEs who must have full ownership of their code and provide observability of it, and Infra, DevOps and SRE teams that empower developers by platforms and a broad variety of tooling to do it. | ||
|
||
Extendability is the part that currently was missed and wasn't supported by the operator from the box, because of the lack of ability to provide external or self-developed internal libsonnet libraries that are required for building of desired dashboard inside of operator. | ||
|
||
To cover that advanced scenario, we've added the ability to provide your JSONnet project with all runtime-required libs to the operator and build it during runtime inside. |
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.
This text is a bit like a "blog post", I'm looking forward to a blog around this. But if it could be rewritten to just focus on the feature itself.
started reviewing, i'll finish it first thing in the morning tomorrow! Looks great so far! |
General comment, can we have a user-deployable example in /examples? I appreciate the unit and e2e tests! |
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.
Looks good to me! I really like the attention you put into this, especially around collision prevention and testing! Thank you!
I'll approve once my comment above is addressed, happy to merge after that!
@HubertStefanski Yes, I'll try to add it today. Do you have suggestion or opinion about the content? |
Not really, as long as the example is easy to understand and deploy, I think that's enough for a user coming from a Google search to try it out. Doesn't need to be anything fancy, just a simple setup for jsonnet dashboards |
@olejeglejeg , just those few EOF errors to fix in your new examples, and I'll approve! |
Relates to #1175