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

Chore(Internal): Add new shared reconciliation functions and migrate GrafanaFolder #1770

Merged
merged 6 commits into from
Dec 16, 2024

Conversation

Baarsgaard
Copy link
Contributor

@Baarsgaard Baarsgaard commented Nov 20, 2024

This PR aims to improve how reconciliation functions are written going forward, starting with the folder controller.

The goal is to reduce duplicated code by moving more logic into matching Grafana instances.
This is achieved by namespacing the client to limit the scope of the search when resources do not specify allowCrossNamespaceImport: true

Additional functions have been added for simplifying building conditions when no matching instances are found.

Lastly, it aligns the existing folder reconcile error logging with other resources

@Baarsgaard
Copy link
Contributor Author

Baarsgaard commented Nov 20, 2024

Since the folder E2E test works, this is unlikely to have introduced any regressions.

@Baarsgaard Baarsgaard marked this pull request as ready for review November 20, 2024 20: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.

The overall approach looks good, I just have a few comments regarding code style!

api/v1beta1/grafanafolder_types.go Outdated Show resolved Hide resolved
api/common_interfaces.go Outdated Show resolved Hide resolved
api/common_interfaces.go Outdated Show resolved Hide resolved
@Baarsgaard Baarsgaard force-pushed the update_folder_reconciliation branch 2 times, most recently from 3692ecf to b112b69 Compare November 25, 2024 22:38
@Baarsgaard
Copy link
Contributor Author

I've applied your suggestions as well as added some test cases for the new list functions for Grafana instances.

@Baarsgaard Baarsgaard requested a review from theSuess November 25, 2024 22:42
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.

Could you rebase your branch? (merged your PR that introduced common fields)

api/v1beta1/grafanafolder_types.go Show resolved Hide resolved
@Baarsgaard Baarsgaard force-pushed the update_folder_reconciliation branch from b112b69 to c66b5e1 Compare November 26, 2024 22:37
@Baarsgaard Baarsgaard changed the title Refactor: Folder reconciliation with new shared functions Chore(Internal): Add new shared reconciliation functions and migrate GrafanaFolder Nov 27, 2024
@Baarsgaard Baarsgaard requested a review from weisdd November 27, 2024 00:15
@Baarsgaard Baarsgaard force-pushed the update_folder_reconciliation branch 2 times, most recently from ca83723 to 598c1c6 Compare November 27, 2024 11:49
@Baarsgaard Baarsgaard force-pushed the update_folder_reconciliation branch from 598c1c6 to 1f715ff Compare November 27, 2024 11:57
@Baarsgaard
Copy link
Contributor Author

@theSuess @weisdd Should be ready for final review now!

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.

Found one last issue in the logic of fetching instances, from an implementation standpoint this looks great!

@pb82 can you take a look as well as this might resolve some issues you're tackling in #1762?

api/v1beta1/common.go Show resolved Hide resolved
controllers/controller_shared.go Outdated Show resolved Hide resolved
@Baarsgaard Baarsgaard requested a review from theSuess November 27, 2024 14:33
@Baarsgaard Baarsgaard force-pushed the update_folder_reconciliation branch from 1f715ff to 52fed3e Compare November 28, 2024 00:45
@chenlujjj
Copy link
Contributor

Can we expect this PR be merged this week? so I can continue to work on #1775. Thanks all!

@weisdd
Copy link
Collaborator

weisdd commented Dec 9, 2024

@chenlujjj at the moment, we're waiting for @pb82 to take a look at the changes. We do not expect to see any significant changes, it's more about double-check we're not breaking anything here.

@theSuess theSuess added this pull request to the merge queue Dec 16, 2024
Merged via the queue into grafana:master with commit 1c2641c Dec 16, 2024
14 checks passed
@Baarsgaard Baarsgaard deleted the update_folder_reconciliation branch December 16, 2024 11:38
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.

4 participants