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

feat: Add folderUID and folderRef in GrafanaDashboard CRD #1601

Conversation

aboulay-numspot
Copy link
Contributor

Aim of the merge request

This MR aims to implement the content of the document discussed in maintainer meeting (currently under #1592).

The features are the following:
If folder is set and none of the other fields is set (backwards compat. case): Create the folder
If folderUID or folderRef is set, don't create the folder
If folderUID or folderRef AND folder is set, the new fields take priority -> don't create the folder

Breaking change

There is no breaking change. We implement on the top of existing GrafanaDashboard CR behavior.

@aboulay-numspot
Copy link
Contributor Author

@theSuess - I know you are interested about this implementation. If it can help you to propose something even better, feel free to propose an evolution in a dedicated MR and close this one.

@aboulay-numspot aboulay-numspot force-pushed the feat/add-folderRef-folderUID-in-grafanadashboard branch from c2cd3f3 to 203ff7c Compare July 4, 2024 15:07
@theSuess
Copy link
Member

theSuess commented Jul 8, 2024

Waiting for #1604 to be merged first as it introduces shared logic we can reuse here

@theSuess theSuess marked this pull request as draft July 8, 2024 09:19
@aboulay-numspot aboulay-numspot force-pushed the feat/add-folderRef-folderUID-in-grafanadashboard branch from 203ff7c to f635d15 Compare July 16, 2024 12:41
@aboulay-numspot aboulay-numspot force-pushed the feat/add-folderRef-folderUID-in-grafanadashboard branch from f635d15 to c2d19da Compare July 16, 2024 12:41
@aboulay-numspot
Copy link
Contributor Author

@theSuess I have updated the GrafanaDashboard CR to match with the interface. To do this, I have added a Condition block into the GrafanaDashboard.Status and add the state update into the Reconciliation loop. I removed the duplicated code with the interface developed in the GrafanaFolder MR (#1604).

Can you have a look on it? I don't understand the full meaning of the Condition object, I would be happy if someone can give me some feedback into this implementation.

I think I'm ready for a review this time.

@aboulay-numspot aboulay-numspot marked this pull request as ready for review July 16, 2024 12:47
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 updating this! I found a few minor nitpicks but looks great overall!

api/v1beta1/grafanadashboard_types.go Outdated Show resolved Hide resolved
api/v1beta1/grafanadashboard_types.go Outdated Show resolved Hide resolved
api/v1beta1/grafanadashboard_types.go Outdated Show resolved Hide resolved
controllers/dashboard_controller.go Outdated Show resolved Hide resolved
@aboulay-numspot aboulay-numspot requested a review from theSuess July 19, 2024 08:36
@theSuess theSuess added this pull request to the merge queue Jul 22, 2024
Merged via the queue into grafana:master with commit e5f6cc5 Jul 22, 2024
14 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.

2 participants