manifests/infrastructure: Setup the platform specific status fields. #1930
Conversation
Commit from: openshift@8b3b375 Using openshift/api@dedfb47b1f (config/v1/types_infrastructure: Add AWS Region, 2019-04-26, openshift/api#300) so the registry can use it as a default for their config resource [1,2]. NOTE: keeps the status.platformType set for all platforms compared to the source commit. [1]: https://github.com/openshift/cluster-image-registry-operator/blob/670539cda460d97e0a3e23f077fdf9dd00d2ccdc/manifests/00-crd.yaml [2]: https://github.com/openshift/cluster-image-registry-operator/blob/670539cda460d97e0a3e23f077fdf9dd00d2ccdc/pkg/apis/imageregistry/v1/types.go#L179-L184
Based on openshift/api#359 that adds the platform specific status for Azure and GCP. This sets the * resource group name for the Azure platform operators that need the location, should use the location of the resource group. * add the project ID and region for the GCP platform this allows operators to create resources for the cluster correctly.
openshift-ci-robot
added
size/L
|
@abhinavdahiya: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
Awesome, thanks! /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, ironcladlou 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 |
|
@abhinavdahiya Well, I didn't realize I was an approver here when I tagged this and now it merged without wide review — sorry. |
|
/cc @coreydaley |
| default: | ||
| config.Status.PlatformStatus.Type = configv1.NonePlatformType | ||
| } | ||
| config.Status.Platform = config.Status.PlatformStatus.Type |
There was a problem hiding this comment.
It seems pointless to have this assignment and the same assignment in the case statements. The assignment in the case statements will be undone by this. Which one we keep depends on whether they are always equal or if there can be skew depending on the platform.
…ormType .status.platformType was being set twice by changes in openshift#1930 This move the assignment to single location after the switch.
| APIServerURL: getAPIServerURL(installConfig.Config), | ||
| APIServerInternalURL: getInternalAPIServerURL(installConfig.Config), | ||
| EtcdDiscoveryDomain: getEtcdDiscoveryDomain(installConfig.Config), | ||
| PlatformStatus: &configv1.PlatformStatus{}, |
There was a problem hiding this comment.
The MCO was reading Platform- is it just enough to switch to PlatformStatus? cc @sgreene570
There was a problem hiding this comment.
@sgreene570 @runcom
This is only set in new 4.2 clusters.
4.1 clusters do not have .status.platformStatus set.
A migration will be required to provide this field to all 4.1 clusters.. or operators can use fields if set and fall back to old field....
Still seeing if a migration will be required as only 4.2 platforms require .status.platformStatus to be set.
There was a problem hiding this comment.
I know, so it's ok to switch to the new platformStatus in 4.2 and live with that as you're doing in this PR?
Still seeing if a migration will be required as only 4.2 platforms require
.status.platformStatusto be set.
👍 awesome, that's what I was looking for as well for MCO specifically, thanks!
8 participants
the API were added in openshift/api#300 and openshift/api#359
Platform specific status fields allow installer a way to provide details about the platform to the operators.
/cc @jhixson74 @patrickdillon
^^-- for the changes
/cc@coreydaley @ironcladlou
^^-- for openshift-ingress and openshift-image-registry operator API for GCP.
Closes #1725