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

Refactor: Dedicated structs for Shared/Common options #1754

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Baarsgaard
Copy link
Contributor

@Baarsgaard Baarsgaard commented Nov 12, 2024

I fully expect this to not get merged in case you want to go in a different direction.
On the other hand, there might be useful code/refactorings in here for smaller PRs

I decided to open this draft in order to better illustrate what I meant with a common options struct, specifically:

  • ResyncPeriod
  • InstanceSelector
  • AllowCrossNamespaceImport

I included the GrafanaCommonStatus struct with a partial implementation as well in the second commit.
The expectation is that this one would help simplify the implementation of Statuses across CRDs (Conditions and more)

Notice that the CRDs have not changed at all up until the third commit, except descriptions and the default value for ResyncPeriod on some resources, and GrafanaNotificationPolicy which now has a AllowCrossNamespaceImport

This should not clash with using Interfaces for ensuring functions are defined for structs.
See commit

I ended up refactoring how Grafana instances are listed and all reconcilers now use the same function.
This allowed me to align the reconciliation loop across all the reconcilers, excluding the Grafanas CRD.

  1. Get the CR from K8s
  2. Check if it should be deleted
  3. Defer a status update
  4. Get matching instances
    • By scoping the client to a namespace if allowCrossNamespaceImport=false
    • When no instances are found, log it and set conditions with NilOrEmptyInstanceListCondition()
  5. Apply resource to Instances
  6. Log errors and update Status accordingly.

With the above alignment, all resources now Implement the same Status object and Common fields, but only Conditions are implemented across, notably Hash and LastResync are missing in a few reconciliation loops.
As a result, GrafanaNotificationPolicy now respects namespace boundaries.

@Baarsgaard Baarsgaard force-pushed the common_opts_structs branch 4 times, most recently from f6c4c23 to b444eda Compare November 17, 2024 13:16
@Baarsgaard Baarsgaard marked this pull request as ready for review November 17, 2024 16:43
@Baarsgaard Baarsgaard changed the title Feature: Dedicated structs for Shared/Common options Refactor: Dedicated structs for Shared/Common options Nov 17, 2024
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.

First of all, I'd like to mention that it's a useful work and thanks a lot for the effort that you've been investing into the project!

For me, the issue with this PR is that it suggests way too many changes at once. Such PRs tend not to get merged, because it's quite difficult to verify that we're not breaking anything. I see that you mentioned the goal of the PR ("to better illustrate what I meant with a common options struct"), but some changes go unexplained. E.g.:

  • Why requeuing after an error is removed from the folder controller? Wouldn't it cause excessive load on API-server?
  • Why resyncPeriod was increased from 5 to 10m?
  • Why DefaultResyncPeriod was removed? Imagine someone runs the operator with incomplete/outdated CR where default value is not set through CR.
  • Why do you want to return nil instead of err in certain functions? And so on.

My suggestion would be:

  • to split this all into a series of PRs (easier to comprehend, even introduction of the struct can go in phases);
  • to explain there all the changes to the default behaviour that you're trying to make. Above, I just gave you an example of questions that appeared in my mind while reading the PR - even if there was a good reason to make those changes, we need to have more context to understand the reasoning.

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