Chore(Internal): Add new shared reconciliation functions and migrate GrafanaFolder
#1770
Conversation
|
Since the folder E2E test works, this is unlikely to have introduced any regressions. |
Baarsgaard
requested review from
NissesSenap,
weisdd,
ishanjainn,
theSuess and
pb82
as code owners
Baarsgaard
force-pushed
the
update_folder_reconciliation
branch
2 times, most recently
from
3692ecf
to
b112b69
Compare
|
I've applied your suggestions as well as added some test cases for the new list functions for Grafana instances. |
Baarsgaard
force-pushed
the
update_folder_reconciliation
branch
from
b112b69
to
c66b5e1
Compare
Baarsgaard
changed the title
GrafanaFolder
Baarsgaard
force-pushed
the
update_folder_reconciliation
branch
2 times, most recently
from
ca83723
to
598c1c6
Compare
Baarsgaard
force-pushed
the
update_folder_reconciliation
branch
from
598c1c6
to
1f715ff
Compare
| @@ -39,3 +39,12 @@ type GrafanaCommonSpec struct { | |||
| // +optional | |||
| AllowCrossNamespaceImport *bool `json:"allowCrossNamespaceImport,omitempty"` | |||
| } | |||
|
|
|||
| // Common Functions that all CRs should implement, excluding Grafana | |||
| // +kubebuilder:object:generate=false | |||
There was a problem hiding this comment.
Just out of curiosity, what kind of object would kubebuilder generate for an interface?
There was a problem hiding this comment.
Nothing, it prints an error:
github.com/grafana/grafana-operator/v5/api/v1beta1:-: invalid type: interface{AllowCrossNamespace() bool; MatchLabels() *k8s.io/apimachinery/pkg/apis/meta/v1.LabelSelector; MatchNamespace() string; ResyncPeriodHasElapsed() bool}
Error: not all generators ran successfully
run `controller-gen object:headerFile=hack/boilerplate.go.txt paths=./... -w` to see all available markers, or `controller-gen object:headerFile=hack/boilerplate.go.txt paths=./... -h` for usage
make: *** [Makefile:90: generate] Error 1
Which is the original reason I didn't have it defined under the v1beta1 package.
Untill I found that specific annotation.
controllers/controller_shared.go
Outdated
| if instanceSelector.MatchLabels == nil { | ||
| return []v1beta1.Grafana{}, nil | ||
| } |
There was a problem hiding this comment.
An empty label selector should match all instances, not none. At least this is the behavior right now, and I think there's value in keeping it that way
There was a problem hiding this comment.
From what I can tell that's not the current behaviour, the existing implementation of GetMatchingInstances(...) exits early.
I left that function alone to not interfere with any other reconciliation loop.
Additionally, instanceSelector is not an optional/nullable field:
There was a problem hiding this comment.
Update: it can be configured with instanceSelector: {}
There was a problem hiding this comment.
Yeah, don't merge this yet, I need to fully understand instanceSelector behaviour before this is ready.
Baarsgaard
force-pushed
the
update_folder_reconciliation
branch
from
1f715ff
to
52fed3e
Compare
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: trueAdditional 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