Create controller to pull serviceprovider data into cosmos from clust… - #4167
Conversation
David Eads (deads2k)
added
the
ai-assisted
David Eads (deads2k)
left a comment
David Eads (deads2k)
left a comment
There was a problem hiding this comment.
huh, I can't request changes on my own PR. interesting.
| needsBaseDomainPrefix := existingCluster.CustomerProperties.DNS.BaseDomainPrefix == "" | ||
|
|
||
| if !needsConsoleURL && !needsBaseDomain && !needsBaseDomainPrefix { | ||
| logger.Info("all properties already set, nothing to sync") |
There was a problem hiding this comment.
don't log in the normal case
| } | ||
|
|
||
| // Check if any of the properties need to be synced | ||
| needsConsoleURL := existingCluster.ServiceProviderProperties.Console.URL == "" |
There was a problem hiding this comment.
len(foo) == 0 is project standard
| } | ||
| } | ||
|
|
||
| if !updated { |
There was a problem hiding this comment.
use a deepequal of the entire thing rather than one by one
|
|
||
| if needsBaseDomain { | ||
| baseDomain := csCluster.DNS().BaseDomain() | ||
| if baseDomain != "" { |
There was a problem hiding this comment.
len check is project standard.
| return utils.TrackError(fmt.Errorf("failed to replace Cluster: %w", err)) | ||
| } | ||
|
|
||
| logger.Info("successfully synced cluster properties from Cluster Service") |
There was a problem hiding this comment.
I don't love it, but it's rare at least.
| } | ||
| } | ||
|
|
||
| func TestClusterPropertiesSyncer_SyncOnce_ClusterNotFound(t *testing.T) { |
There was a problem hiding this comment.
not a very useful test.
|
lol. AI forgot to add it to main to actually run. |
552c256 to
81a3555
Compare
|
What's here looks good. I wonder if this controller could be extended to also populate the |
|
/hold cancel |
Good thought. We've actually got PRs stacked up that allow us to determine this directly from azure in the backend. I think it's #4193. |
|
/lgtm |
…er-service We will use this for the read path of frontend since the information is already asynchronous, so some delay is ok. We must get the backend change to prod first though, becuase otherwise we can have a frontend updated and backend not updated and be unable to ever get these values.
81a3555 to
5702af3
Compare
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, mbarnes The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test all |
3 participants
…er-service
We will use this for the read path of frontend since the information is already asynchronous, so some delay is ok. We must get the backend change to prod first though, becuase otherwise we can have a frontend updated and backend not updated and be unable to ever get these values.
I have not yet reviewed this, but I think it's useful to illustrate a direction we're going to take for removing cluster-service from the read path so that we can move cluster-service creation to the backend.
/hold until I've reviewed (or if someone wants to review an AI created PR before me, but I wouldn't ask it)
opening this now so I don't lose it over the weekend.