CLOUDP-316083: Third Party Integrations Controller #2313
Conversation
josvazg
changed the title
josvazg
force-pushed
the
CLOUDP-316083/integrations-ctlr
branch
14 times, most recently
from
105c5be to
d35b1f4
Compare
josvazg
force-pushed
the
CLOUDP-316083/integrations-ctlr
branch
4 times, most recently
from
67fd52f to
0fa07a0
Compare
josvazg
force-pushed
the
CLOUDP-316083/integrations-ctlr
branch
2 times, most recently
from
8375d68 to
fc928bf
Compare
josvazg
force-pushed
the
CLOUDP-316083/integrations-ctlr
branch
from
fc928bf to
38ac42f
Compare
josvazg
force-pushed
the
CLOUDP-316083/integrations-ctlr
branch
from
38ac42f to
b653cd5
Compare
| Watches( | ||
| &akov2.AtlasProject{}, | ||
| handler.EnqueueRequestsFromMapFunc(h.integrationForProjectMapFunc()), | ||
| builder.WithPredicates(integrationPredicates), |
There was a problem hiding this comment.
we want to watch for generation changes only here.
| Watches( | ||
| &akov2.AtlasProject{}, | ||
| handler.EnqueueRequestsFromMapFunc(h.integrationForProjectMapFunc()), | ||
| builder.WithPredicates(integrationPredicates), |
There was a problem hiding this comment.
the global variable integrationPredicates obfuscates what predicates we're having so reading through it makes it hard to reason about what predicates are actually applied. I suggest using predicate.GenerationChangedPredicate{} and predicate.ResourceVersionChangedPredicate{} to make it explicit and readable to the reader.
In addition you risk faulty behaviour in case controller-runtime decides to have internal state inside the predicate struct. I'd be careful reusing it.
There was a problem hiding this comment.
Should I use a function instead?
the reason to introduce a variable is we are setting the same predicates in several places. We set one predicate in 3 or 4 places and a different one on another. To avoid mistakes, all same occurrences should be the same reference in code. If a variable is not good, a function returning a fresh instance every time might work instead.
There was a problem hiding this comment.
I will just drop the var and funcs in the end.
| &corev1.Secret{}, | ||
| handler.EnqueueRequestsFromMapFunc(h.integrationForSecretMapFunc()), | ||
| builder.WithPredicates(integrationPredicates), | ||
| ). |
There was a problem hiding this comment.
really just a nit: rather than watching twice for Secret, we can just make one combined enqueue function, gathering both from the AtlasThirdPartyIntegrationCredentialsIndex and AtlasThirdPartyIntegrationBySecretsIndex index.
There was a problem hiding this comment.
Those secrets are semantically different. One if for operator credentials, and the other are for integration credentials. The indexers are totally different, even if they map secret<-> integration.
I will try to combine, but it might not be that easy.
| return h.update(ctx, currentState, req, integrationSpec) | ||
| } | ||
| return result.NextState( | ||
| state.StateCreated, |
There was a problem hiding this comment.
| state.StateCreated, | |
| state.StateUpdated, |
There was a problem hiding this comment.
alternative: pass a nextState var as well, callers would be then:
func (h *AtlasThirdPartyIntegrationHandler) HandleInitial(ctx context.Context, integration *akov2next.AtlasThirdPartyIntegration) (ctrlstate.Result, error) {
return h.upsert(ctx, state.StateInitial, state.StateCreated, integration)
}
func (h *AtlasThirdPartyIntegrationHandler) HandleCreated(ctx context.Context, integration *akov2next.AtlasThirdPartyIntegration) (ctrlstate.Result, error) {
return h.upsert(ctx, state.StateCreated, state.StateUpdated, integration)
}
func (h *AtlasThirdPartyIntegrationHandler) HandleUpdated(ctx context.Context, integration *akov2next.AtlasThirdPartyIntegration) (ctrlstate.Result, error) {
return h.upsert(ctx, state.StateUpdated, state.StateUpdated, integration)
}
There was a problem hiding this comment.
I think I go for the simpler static fix.
There was a problem hiding this comment.
I think I go for the simpler static fix.
There was a problem hiding this comment.
Actually, the next step is best, as if it was already created and it did not change, we want it to stay saying created.
| } | ||
| if err != nil { | ||
| return result.Error( | ||
| state.StateInitial, |
There was a problem hiding this comment.
| state.StateInitial, | |
| state.StateDeletionRequested |
| return ctrlstate.NewStateReconciler( | ||
| handler, | ||
| ctrlstate.WithCluster[akov2next.AtlasThirdPartyIntegration](c), | ||
| ctrlstate.WithReapplySupport[akov2next.AtlasThirdPartyIntegration](reapplySupport), |
There was a problem hiding this comment.
this looks very very tasty, great job! 👏
Signed-off-by: jose.vazquez <[email protected]>
Summary
Internal-only implementation for the integrations controller
Proof of Work
TBD including new tests
Checklist
Reminder (Please remove this when merging)