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 proposal for GrafanaLibraryPanel CRD #1611

Merged
merged 4 commits into from
Nov 26, 2024

Conversation

diurnalist
Copy link
Contributor

this adds a proposal for a GrafanaLibraryPanel CRD, which would enable the operator to manage Library Panels similar to other assets. the Library Panel behaves similar as a Dashboard; it has a JSON model and can optionally be associated to a Folder.

full disclosure: we actually have a patch that we are using in production that implements the proposal documented here. We don't have chainsaw tests for it though; I think that would be one possible next step. still, open to discussion about the approach. we opted to implement this with high parity with how Dashboards worked because we reasoned folks might want to manage the JSON models for both in similar ways, be that as config maps, or nested on the CR itself, or external URL.

Copy link
Collaborator

@NissesSenap NissesSenap left a comment

Choose a reason for hiding this comment

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

Really nice feature to get, thanks for the initial design.

Personally I think it would be interesting to discuss how the dashboard would reference lib panels. I think it would be reasonable to do it in a similar way as we do here: #1604

Just because we talk about the design, doesn't mean that we have to have this feature to be a part of the initial implementation, but it might have an impact on how we make the implementation.

How do you feel about it?

Copy link

This PR hasn't been updated for a while, marking as stale

@github-actions github-actions bot added the stale label Aug 16, 2024
@github-actions github-actions bot closed this Aug 23, 2024
@applike-ss
Copy link

Can we have this re-opened? This seems like a good thing to have.

@NissesSenap NissesSenap reopened this Oct 7, 2024
@github-actions github-actions bot removed the stale label Oct 8, 2024
Copy link

github-actions bot commented Nov 8, 2024

This PR hasn't been updated for a while, marking as stale

@github-actions github-actions bot added the stale label Nov 8, 2024
@applike-ss
Copy link

Any chance this gets merged, so people can start to work on it?

@theSuess theSuess removed the stale label Nov 8, 2024
@NissesSenap
Copy link
Collaborator

@applike-ss i feel that we need more details in - the suggestion as I asked for in my above comment. But I haven't gotten any reply from @diurnalist . If you are up for it to clarify the document I would love to see a new PR around this design.

@diurnalist
Copy link
Contributor Author

hey - I'm still interested in getting this done since we already have most of the code in our fork. I haven't had as much time to focus on it b/c my priorities have moved on to other things at $JOB and the operator fork we have is working fine.

i might have some cycles this weekend and could donate some to finishing the proposal to address the above questions.

@diurnalist
Copy link
Contributor Author

@NissesSenap, all, I have updated the proposal to include two ways to reference panels from w/in dashboards. One way, via static UID, is more friendly if the model is that end-users pick and choose from library panels in the UI and then export for persistence to code. The other way, dynamic reference, allows for administrators to provision library panels and dashboards in concert w/ one another and have them be linked up correctly, similar to how the Datasource mapping features work right now. This would also provide a mechanism to understand dependencies between dashboards and library panels in the Operator.

"id": 2,
"libraryPanel": {
"uid": "${LP_GRPC_SERVER_SUCCESS_RATE}",
"name": "gRPC Server Success Rate"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tbh I am not sure what this .name field is even used for. It could possibly also be dynamically injected, and we could have this just be "libraryPanel": "${LP_GRPC_SERVER_SUCCESS_RATE}", but that might be a bit too surprising of an interface.

Copy link
Member

Choose a reason for hiding this comment

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

I tested importing this with an arbitrary value, and it seems to work, so I guess we can just ignore that field

Copy link
Member

@theSuess theSuess left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to update this. From my point of view, this looks like a reasonable approach.

Would love to hear the other maintainers thoughts on this as well

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.

Thanks for preparing the document! I think it's well-written and suggests a good approach on the new resource.
Once someone starts working on the implementation, we'll see if anything needs to be corrected, but, overall, it should be a good start for sure.

@weisdd weisdd added this pull request to the merge queue Nov 26, 2024
Merged via the queue into grafana:master with commit 7ab226c Nov 26, 2024
11 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.

5 participants