-
Notifications
You must be signed in to change notification settings - Fork 409
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 parentFolderRef field in GrafanaFolder #1604
feat: add parentFolderRef field in GrafanaFolder #1604
Conversation
d5a7194
to
37143be
Compare
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.
Thanks for the PR, left two notes on the implementation details but the approach is solid 👍
@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. I think it is one step closer to the Grafana t-shirt 😄 |
@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 :) |
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 |
@theSuess - Smart, you are using an interface to hide the difference of naming between the GrafanaFolder (with parentFolderUID/parentFolderRef) and GrafanaAlertRuleGroup (with folderUID/folderRef). 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.
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). |
bc40ad4
to
907f924
Compare
88be430
to
ab03f08
Compare
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.