CONSOLE 2919: code review adjustments for managed cluster action/views#617
CONSOLE 2919: code review adjustments for managed cluster action/views#617florkbr wants to merge 1 commit intoopenshift:master-multi-cluster-featurefrom
Conversation
Addresses @jhadvig comments on openshift#602.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: florkbr The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
|
|
/retest |
|
@florkbr: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
|
See #619 for |
TheRealJon
left a comment
TheRealJon
left a comment
There was a problem hiding this comment.
This is going to conflict heavily with #612. I've commented on most of the obvious conflicts that I saw. I think you can probably revert the changes to all of the following files since #612 has duplicate or similar updates:
- pkg/console/operator/sync_v400.go
- pkg/console/subresource/configmap/configmap.go
- pkg/console/subresource/configmap/configmap_test.go
There will also be conflicts with the file renames in pkg/console/subresource/managedclusterview and pkg/console/subresource/managedclusteraction. See my comments on that. I think we should keep those as separate modules per API, but you might want to revert your changes in this PR regarding those modules, then wait for #612 to merge and make any file name updates after rebasing.
| canMountManagedClusterConfig, managedClusterConfigErrReason, managedClusterConfigErr := co.SyncManagedClusterConfigMap(ctx) | ||
| statusHandler.AddConditions(status.HandleProgressingOrDegraded("ManagedClusterConfigSync", managedClusterConfigErrReason, managedClusterConfigErr)) |
There was a problem hiding this comment.
I address conditionally adding the managed cluster config file name to the console config in #612.
| activeConsoleRoute *routev1.Route, | ||
| useDefaultCAFile bool, | ||
| inactivityTimeoutSeconds int, | ||
| pluginsEndpoingMap map[string]string) (consoleConfigmap *corev1.ConfigMap, unsupportedOverridesHaveMerged bool, err error) { |
There was a problem hiding this comment.
I have implemented similar changes to this file and the related unit test file in #612.
| kind: ConfigMap | ||
| metadata: | ||
| namespace: openshift-config-managed | ||
| namespace: openshift-console |
| kind: ConfigMap | ||
| metadata: | ||
| namespace: openshift-config-managed | ||
| namespace: openshift-console |
| @@ -1,4 +1,4 @@ | |||
| package managedclusterview | |||
| package managedcluster | |||
There was a problem hiding this comment.
I'd prefer to keep MCV/MCA subresource modules separated by kind.
|
Closing as Jon delivered these in #630. Thanks Jon! |
2 participants
Addresses @jhadvig comments on #602.
See https://issues.redhat.com/projects/CONSOLE/issues/CONSOLE-2919
CC @kdoberst @TheRealJon