Add CRUD for new type: ServiceProviderCluster for controllers #3715
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k 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 |
deads2k
force-pushed
the
cs-81-controller-staus
branch
from
1386ff3 to
0dbf7a7
Compare
|
if I broke e2e that badly and didn't trigger the simulation tests, something went seriously wrong. |
…urceID ResourceID is tolowered and then swap / for | to make a valid cosmosID.
deads2k
force-pushed
the
cs-81-controller-staus
branch
from
0dbf7a7 to
5b4d2ba
Compare
internal/api/types_cosmosdata.go
Outdated
|
|
||
| func (o *CosmosMetadata) GetCosmosData() CosmosData { | ||
| return CosmosData{ | ||
| CosmosUID: strings.ReplaceAll(strings.ToLower(o.ResourceID.String()), "/", "|"), |
There was a problem hiding this comment.
I am not familiar with this. Why is this replacement needed?
There was a problem hiding this comment.
cosmos uses a REST API, which means that IDs that contain slashes cause problems with URL handling. We chose | because that is a delimiter that is not allowed inside of an ARM resource ID because it is a separator for multiple resource IDs.
Note to self, be sure to add this explanation as a golang comment for future readers.
| ResourceTypeDisplay = "Hosted Control Plane (HCP) OpenShift Clusters" | ||
| ) | ||
|
|
||
| var ( |
There was a problem hiding this comment.
A question that is broader than the scope of this MR:
Some of the API types are fully internal, not exposed to the end-user as Azure resource types. Is there some reason we are defining those like the one in the case of this MR as subresources of the exposed ones? The same
There was a problem hiding this comment.
Yes there is a reason. Our data is cosmos is stored in a logical directory structure based on prefix. So we have logically, /subscription/subid/resourcegroup/rgname/providers/us/clusters/cluster-name, etc. And we rely on this structure for gathering debugging data (show me all the data under this directory) and we rely on this structure for cleanup (we deleted this nodepool, remove everythign related to it).
for non-exposed resourcetypes, we are one data migration and two weeks away from changing the name at any instant in time from now to eternity because they're entirely internal details with three clients: admin API, backend, frontend.
This allows controllers to coordinate values
deads2k
force-pushed
the
cs-81-controller-staus
branch
from
5b4d2ba to
2e0e058
Compare
|
comments addressed by adding details to code. |
When bringing controllers from cluster-service, they need a place to save data. To avoid conflicting with users and to allow future optimistic concurrency, this adds a new type to store in cosmos.
It builds on #3713 (green CI) and creates the first example of where we want to take all our stored types: GenericDocument storage that uses an InternalAPI type that is fully convertible between internal, cluster-service, cosmos, and ARM types. That leverages our new hub type used throughout the frontend without any conversion.
/assign @mbarnes
/cc @miguelsorianod