(feat): Add migration logic for odhdashboardconfig #1709
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
AjayJagan
force-pushed
the
dashboard-migration-script
branch
from
704bbb2 to
28077d3
Compare
AjayJagan
force-pushed
the
dashboard-migration-script
branch
from
28077d3 to
93f7ae4
Compare
Signed-off-by: AjayJagan <ajaganat@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
AjayJagan
force-pushed
the
dashboard-migration-script
branch
from
93f7ae4 to
7c3be1d
Compare
AjayJagan
force-pushed
the
dashboard-migration-script
branch
from
e84b51b to
f10e296
Compare
|
/test opendatahub-operator-e2e |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1709 +/- ##
==========================================
- Coverage 23.68% 23.37% -0.31%
==========================================
Files 169 169
Lines 11625 11776 +151
==========================================
Hits 2753 2753
- Misses 8598 8749 +151
Partials 274 274 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
AjayJagan
force-pushed
the
dashboard-migration-script
branch
from
f10e296 to
18b3133
Compare
| @@ -615,3 +615,179 @@ func cleanupModelControllerLegacyDeployment(ctx context.Context, cli client.Clie | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| // TODO: to be removed: https://issues.redhat.com/browse/RHOAIENG-21080 | |||
| func PatchOdhDashboardConfig(ctx context.Context, cli client.Client) error { | |||
There was a problem hiding this comment.
Probably this function should also check the current and previous version to determine if the patch should be applied ? something like https://github.com/opendatahub-io/opendatahub-operator/blob/main/pkg/upgrade/upgrade.go#L503
There was a problem hiding this comment.
yep it can be done, although I may need some guidance on which versions should be patched
@Gkrumbach07 ^
pkg/upgrade/upgrade.go
Outdated
| } | ||
|
|
||
| for _, cr := range dashboardConfigs.Items { | ||
| if !cluster.IsNotReservedNamespace(&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: cr.GetNamespace()}}) { |
There was a problem hiding this comment.
is this required ? my understanding is that there is only one OdhDashboardConfig per cluster so I thinnk it should be patched, regardless of the namespace ?
There was a problem hiding this comment.
I have changed the logic to handle the singleton case
pkg/upgrade/upgrade.go
Outdated
| log.Info("Found CR, applying patch", "namespace", cr.GetNamespace(), "name", cr.GetName()) | ||
|
|
||
| patch := cr.DeepCopy() | ||
| updates := map[string][]any{ |
There was a problem hiding this comment.
nip: since the updates map is always the same, can't this be just a var ? not sure I see the benefits of having the values returned by functions
There was a problem hiding this comment.
var would do fine 😄
Signed-off-by: AjayJagan <ajaganat@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
AjayJagan
force-pushed
the
dashboard-migration-script
branch
from
10274b4 to
b0ea636
Compare
rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
|
@AjayJagan: The following test failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
Signed-off-by: AjayJagan ajaganat@redhat.com
rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
Description
The dashboard team has asked for a temporary fix which is to patch the odhdashboardconfig CR if it doesnot have an entry of
notebookSizesormodelServerSizes.JIRA: https://issues.redhat.com/browse/RHOAIENG-19058
Note that these changes will be removed once dashboard fixes it on their side.
jira to track removal: https://issues.redhat.com/browse/RHOAIENG-21080
How Has This Been Tested?
modelServerSizesandnotebookSizesgetting added if they are missed or if the array is emptyScreenshot or short clip
Merge criteria