Add MC transit gateway reconciler #69
Conversation
Co-authored-by: Jose Armesto <github@armesto.net>
Co-authored-by: Jose Armesto <github@armesto.net>
There was a problem hiding this comment.
Left a few comments.
Apart from that, on this repository I tried to avoid mixing dependencies between packages as much as possible. For example, the aws package didn't depend on any CAPI/CAPA/k8s library. That's why there is a Cluster object defined on this repository.
I think there is value on this isolation, that will help us later on when upgrading these libraries. Thoughts?
| if len(gateways) == 0 { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
I think a potential client of this function would expect an error when trying to find an object that doesn't exist. What do you think about returning error on that case?
There was a problem hiding this comment.
It'll probably be added later. As it is right now it isn't needed as it's private.
| defer func() { | ||
| _ = r.clusterClient.UpdateStatus(ctx, cluster) | ||
| }() |
There was a problem hiding this comment.
Functions like Addfinalizer call the k8s API right away, sending a patch request to modify the object. So I think it's odd that we are mixing both approaches here. On one hand, we send request to the API right away to do the modification, while using defer we update the object when the function returns. It's fine, but I'd prefer to use the same approach for all cases.
There was a problem hiding this comment.
I'm not too sure about this one. This updates the conditions in the status and those can be overwritten in the course of reconciling (for example it starts as InProgress, but can transition to error or completed in the same loop). So I think it makes sense to do it with defer here.
For the finalizer though it does make sense to just change that and nothing else. In fact we explicitly don't want to change anything else because we shouldn't be changing the desired state. I guess we can convert the status updates to be done the same way, but I'm not too sure about it
There was a problem hiding this comment.
That makes sense, let's keep it like you have now then 👍
Co-authored-by: Jose Armesto <github@armesto.net>
|
I've changed the client to take |
What this PR does / why we need it
Towards giantswarm/roadmap#1827 and https://github.com/giantswarm/giantswarm/issues/25875
This is only the first out of 3 reconcilers that implement he aws-network-topology-operator, which is why it isn't yet added in
main.go, so after merging this PR it still won't be enabled.Checklist