CLOUDP-238527: Atlas streams instance controller #1529
Conversation
helderjs
requested review from
s-urbaniak,
josvazg,
igor-karpukhin and
roothorp
s-urbaniak
force-pushed
the
atlas-streams
branch
from
48530ea to
10dd8b2
Compare
| for i := range ops.NoOp { | ||
| akoStreamConnection := ops.NoOp[i] | ||
| ctx.EnsureStatusOption( | ||
| status.AtlasStreamInstanceAddConnection( | ||
| akoStreamConnection.Spec.Name, | ||
| common.ResourceRefNamespaced{ | ||
| Name: akoStreamConnection.Name, | ||
| Namespace: akoStreamConnection.Namespace, | ||
| }, | ||
| ), | ||
| ) | ||
| } |
There was a problem hiding this comment.
I do not understand no ops, we are doing something so, are they really no ops?
There was a problem hiding this comment.
Those are connections that didn't change anything in a reconcile loop, we still need to push them in the status.
| func hasStreamConnectionChanged( | ||
| streamConnection *akov2.AtlasStreamConnection, | ||
| atlasStreamConnection *admin.StreamsConnection, | ||
| mapper streamConnectionMapper, |
There was a problem hiding this comment.
Side comment:
With a translation layer this always compares pears to pears, because the admin type would have been already translated before getting here. Eg:
There was a problem hiding this comment.
I still haven't checked your PR, but yes, if we had a middle layer that wouldn't be needed. However, I'm wondering if the transformation back and forth from both sides worth it.
| return r.terminate(workflowCtx, workflow.AtlasAPIAccessNotConfigured, err) | ||
| } | ||
| workflowCtx.SdkClient = atlasClient | ||
| workflowCtx.OrgID = orgID |
There was a problem hiding this comment.
Are we sure we use this?
In the PoC I could not see usages, so I removed it.
There was a problem hiding this comment.
In this resource reconciliation, we don't need to be aware of the org ID, but workflow context is built in multiple steps and I prefer not to leave it incomplete because the org id is not required by streams.
s-urbaniak
force-pushed
the
atlas-streams
branch
from
72c0247 to
0ca4a70
Compare
|
we need to do some more logistics around helm |
All Submissions:
closes #XXXXin your comment to auto-close the issue that your PR fixes (if there is one).An integration test will be added in an upcoming PR, the connection controller needs to be finished first as the controllers communicate with each other.