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 parentFolderRef field in GrafanaFolder #1604

Merged

Conversation

aboulay-numspot
Copy link
Contributor

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.

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 the PR, left two notes on the implementation details but the approach is solid 👍

controllers/grafanafolder_controller.go Outdated Show resolved Hide resolved
controllers/grafanafolder_controller.go Outdated Show resolved Hide resolved
@aboulay-numspot
Copy link
Contributor Author

aboulay-numspot commented Jul 5, 2024

@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: 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 😄

@theSuess theSuess changed the title feat : add parentFolderRef field in GrafanaFolder feat: add parentFolderRef field in GrafanaFolder Jul 8, 2024
@theSuess
Copy link
Member

theSuess commented Jul 8, 2024

@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
Copy link
Member

theSuess commented Jul 8, 2024

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
Copy link
Contributor Author

aboulay-numspot commented Jul 8, 2024

@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).

@theSuess theSuess force-pushed the feat/add-folderRef-in-grafanaFolder branch from bc40ad4 to 907f924 Compare July 15, 2024 09:34
@theSuess theSuess force-pushed the feat/add-folderRef-in-grafanaFolder branch from 88be430 to ab03f08 Compare July 15, 2024 10:06
@theSuess theSuess enabled auto-merge July 15, 2024 10:07
@theSuess theSuess added this pull request to the merge queue Jul 15, 2024
Merged via the queue into grafana:master with commit 43ed0d2 Jul 15, 2024
12 of 13 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