search

grafana / grafana-operator

An operator for Grafana that installs and manages Grafana instances, Dashboards and Datasources through Kubernetes/OpenShift CRs
https://grafana.github.io/grafana-operator/
Apache License 2.0
863 stars 384 forks source link

feat: add parentFolderRef field in GrafanaFolder #1604

Closed aboulay-numspot closed 1 month ago

aboulay-numspot commented 2 months ago

Aim of the merge request

Implement the parentFolderRef functionality in the GrafanaFolder CRD.

Breaking change

There is no breaking change in this MR

Related documents

Additional comments

I have made few tests on moving the folder with dashboard inside by changing the parentFolderRef/parentFolderUID. I have adapted the implementation based on #1600 changes.

aboulay-numspot commented 2 months ago

@theSuess Normally, this should be fine now. I have mutualized the content of the field for GrafanaFolder and GrafanaAlertRuleGoroup CR into the Shared Controller and change the position of the validation to the kubebuilder part.

If I can get help to test regression into the GrafanaAlertRuleGroup, it would be appreciated.

:warning: Warning: this will require a reinstallation of the CRD and the operator with make start-kind to test the kubebuilder condition.

I think it is one step closer to the Grafana t-shirt :smile:

theSuess commented 2 months ago

@aboulay-numspot I added a commit to try out a different pattern. I've asked the other maintainers to take a look if this suits our project, but I'd like to hear your take on this as well. If it doesn't make sense, we can always revert this commit :)

theSuess commented 2 months ago

ignore the CI failure for now, kube-builder doesn't like the interface living in the same package - this is something we can fix once we've decided on the path to take

aboulay-numspot commented 2 months ago

@aboulay-numspot I added a commit to try out a different pattern. I've asked the other maintainers to take a look if this suits our project, but I'd like to hear your take on this as well. If it doesn't make sense, we can always revert this commit :)

@theSuess - Smart, you are using an interface to hide the difference of naming between the GrafanaFolder (with parentFolderUID/parentFolderRef) and GrafanaAlertRuleGroup (with folderUID/folderRef). Initially, I got a similar approach to check folderUID/folderRef (and equivalent) but I was thinking there is too many parameter in my function and it does not look well.

Something important to notice is that now, if we are using this function, we need to have folderRef() and folderUID() set to use this method.

The implementation looks good to me. Thank you for this implementation.

ignore the CI failure for now, kube-builder doesn't like the interface living in the same package - this is something we can fix once we've decided on the path to take

For the e2e test, I can let it go. However, I think we can do something for the linter (or add a comment to disable the behavior on this line).