New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Azure Stack separate assets #96
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can simplify this a bit, please see my comments below
pkg/cloud/cloud.go
Outdated
@@ -16,16 +17,23 @@ import ( | |||
// changes in their spec. However you can extend any resource spec with | |||
// values not specified in the provided source resource. These changes | |||
// would be preserved. | |||
func GetResources(platform configv1.PlatformType) []client.Object { | |||
func GetResources(platform configv1.PlatformType, platformStatus *configv1.PlatformStatus) []client.Object { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
platform
comes from platformStatus
, we don't need to path both in explicitly, let's simplify this
func GetResources(platform configv1.PlatformType, platformStatus *configv1.PlatformStatus) []client.Object { | |
func GetResources(platformStatus *configv1.PlatformStatus) []client.Object { |
@@ -140,7 +140,7 @@ func (r *CloudOperatorReconciler) Reconcile(ctx context.Context, _ ctrl.Request) | |||
return ctrl.Result{}, err | |||
} | |||
|
|||
if err := r.sync(ctx, config); err != nil { | |||
if err := r.sync(ctx, config, infra.Status.PlatformStatus); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could add the PlatformStatus to the config
to replace the platform type there?
…ify GetResources function.
/approve |
pkg/cloud/azurestack/azurestack.go
Outdated
utilruntime.Must(err) | ||
} | ||
|
||
// GetResources returns a list of AWS resources for provisioning CCM in running cluster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// GetResources returns a list of AWS resources for provisioning CCM in running cluster | |
// GetResources returns a list of Azure resources for provisioning CCM in running cluster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Fedosin, JoelSpeed 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 |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
7 similar comments
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/test e2e-azure-ccm |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/test e2e-azure-ccm |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
4 similar comments
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
We are aware that the tests are not currently passing because of a relatively major bug in the Azure CCM when running on OpenShift (this is to do with how openshift is deployed on Azure). We believe we have a working solution, and at this point believe this PR is correct. We will continue to work on the bugs over the next few days and hope to have the Azure/ASH CCM stable by the end of next week. /override ci/prow/e2e-azure-ccm |
@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/e2e-azure-ccm In response to this:
Instructions 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. |
Accepting this feature PR after feature freeze because this is the last critical piece in delivering an urgent feature. The work was done but delayed due to a strong desire to test more thoroughly and coordinate with other teams. Overriding bugzilla/valid-bug label. |
/retest-required |
There is plenty of changes between public Azure and Stack one. With current substitution approach it is not convenient to handle it. Extracted to separate module.