Refactor transit gateway status #1961
Conversation
k8s-ci-robot
added
area/provider/ibmcloud
✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
k8s-ci-robot
added
the
size/L
cloud/scope/powervs_cluster.go
Outdated
| switch networkType { | ||
| case powervsNetworkConnectionType: | ||
| if s.IBMPowerVSCluster.Status.TransitGateway.PowerVSConnection != nil { | ||
| s.IBMPowerVSCluster.Status.TransitGateway.PowerVSConnection.ControllerCreated = resource.ControllerCreated |
There was a problem hiding this comment.
why do we need to assign seperatly , why can't we assign directly as like in else case?
There was a problem hiding this comment.
Agree, removed these nil check and directly assigned, ptal!
| if err != nil { | ||
| return false, err | ||
| } | ||
| s.SetTransitGatewayStatus(tg.ID, ptr.To(false), powerVSConn, vpcConn) |
There was a problem hiding this comment.
Instead of moving this to isTransitGatewayExists() can we just move it couple of lines above just after checking tg!=nil? and then proceed towards checking connection/creating connections.
Like most of other functions, it would be good if we keep SetStatus mostly under ReconcileResource function
There was a problem hiding this comment.
Also how about calling checking TG status and later checking TG connections separately from here, Rather than embedding both inside checkAndUpdateTransitGateway?
There was a problem hiding this comment.
But while creating I am setting(need to) the status immediately after create call in s.createTransitGateway(), so I feel its ok to keep inside isTransitGatewayExists() because that func meant to process the existing resource, so when we set controllercreated to false, it would be good if we keep it inside that func.
Rather than embedding both inside checkAndUpdateTransitGateway?
Then the main Reconcile would be too long if I add these three calls and check the error, it's better to keep them in this func IMO.
| return transitGateway, nil | ||
| } | ||
|
|
||
| // checkAndUpdateTransitGateway checks given transit gateway's status and its connections. | ||
| // if update is set to true, it updates the transit gateway connections too if it is not exist already. | ||
| func (s *PowerVSClusterScope) checkAndUpdateTransitGateway(transitGateway *tgapiv1.TransitGateway, update bool) (bool, *infrav1beta2.ResourceReference, *infrav1beta2.ResourceReference, error) { | ||
| func (s *PowerVSClusterScope) checkAndUpdateTransitGateway(transitGateway *tgapiv1.TransitGateway) (bool, error) { | ||
| requeue, err := s.checkTransitGatewayStatus(transitGateway) |
There was a problem hiding this comment.
Just a thought, I feel checkTransitGatewayStatus() is getting confused for checking Status of TG from IBMPowerVSCluster resource.
checkTransitGatewayStatus() is actually checking status of TG in cloud, Can we rename the function if possibel or if you thinks its better.
There was a problem hiding this comment.
It seems whenever we are checking the cloud we are following this format check<something> and when we query from cluster status we are using Get<something>, I think it's ok.
dharaneeshvrd
force-pushed
the
tg-connections
branch
2 times, most recently
from
cff4218 to
a9a9756
Compare
cloud/scope/powervs_cluster.go
Outdated
| @@ -1886,53 +1847,55 @@ func (s *PowerVSClusterScope) checkTransitGatewayConnectionStatus(con tgapiv1.Tr | |||
| } | |||
|
|
|||
| // createTransitGatewayConnection creates transit gateway connection. | |||
There was a problem hiding this comment.
can we elaborate the description accordingly as we are also setting the status?
cloud/scope/powervs_cluster.go
Outdated
| } | ||
|
|
||
| return powerVSConn.ID, vpcConn.ID, nil | ||
| return nil | ||
| } | ||
|
|
||
| // createTransitGateway create transit gateway. |
There was a problem hiding this comment.
can we elaborate the description accordingly to specify about setting the status?
dharaneeshvrd
force-pushed
the
tg-connections
branch
from
a9a9756 to
89a7c50
Compare
|
Addressed the comments @Amulyam24 ptal! |
k8s-ci-robot
added
the
lgtm
|
LGTM! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dharaneeshvrd, mkumatag The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What this PR does / why we need it:
#1960
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
/area provider/ibmcloud
Release note: