pkg/upgrade: remove watson-studio dashboard application #959

ykaliuta · 2024-04-08T14:30:18Z

Remove deprecated dashboard resources, watson-studio

Description

Jira: https://issues.redhat.com/browse/RHOAIENG-4764

How Has This Been Tested?

Install odh-2.9.0
Deploy odh-manifests/dashboard/overlays/apps/base/watson
Check odhapplications.dashboard.opendatahub.io, odhdocuments.dashboard.opendatahub.io and odhquickstarts.console.openshift.io in opendatahub namespace, watson objects exist
Deploy PR version
Restart operator pod
Observe that watson objects do not exist anymore

Merge criteria:

The commits are squashed in a cohesive manner and have meaningful messages.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

openshift-ci · 2024-04-08T14:30:23Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

ykaliuta · 2024-04-08T16:57:43Z

/test all

zdtsw · 2024-04-09T13:44:46Z

pkg/upgrade/upgrade.go

@@ -254,6 +266,33 @@ func UpdateFromLegacyVersion(cli client.Client, platform deploy.Platform, appNS
 	return nil
 }

+func jobGetDashboardWatsonResources(ns string) []JobSpec {
+	pathName := []string{"metadata", "name"}


how you feel name it as

Suggested change

pathName := []string{"metadata", "name"}

metaDataName := []string{"metadata", "name"}

specAppName := []string{"spec", "appName"}

or maybe not use variable but directly set it ?

how you feel name it as

Fine. Naming is a BIG question here, any suggestion is welcome!

or maybe not use variable but directly set it ?

it's repeated several times (except "metadata", "name" here, leftover from the previous version), I would prefer to keep them in variables since it's shorter. But no problem to change if you think it's better.

ok, then lets get the change for the variable names instead of hardcode path

ok, then lets get the change for the variable names instead of hardcode path
Updated. I just kept "metadata" lowercase as the field name.

zdtsw · 2024-04-09T15:06:03Z

have done test on the code, looks good for me

bartoszmajsak · 2024-04-09T15:30:04Z

pkg/upgrade/upgrade.go

+type JobSpec struct {
+	Gvk       schema.GroupVersionKind
+	Namespace string
+	// path to the field, like "metadata", "name"
+	Path []string
+	// set of values for the field to match object, any one matches
+	Values []string
+}


Is it a JobSpec or ResourceSpec? It does not carry any job-specific traits (yet). Perhaps in #957 we could split ResourceSpec to carry what you defined here + matcher and expand on the JobSpec with retry and actual func doing the work. WDYT?

bartoszmajsak · 2024-04-09T15:33:38Z

pkg/upgrade/upgrade.go

+	return errors.ErrorOrNil()
+}
+
+func deleteOneJob(ctx context.Context, c client.Client, job JobSpec) error {


if https://github.com/opendatahub-io/opendatahub-operator/pull/959/files#r1557861353 then this is deleteResource

if https://github.com/opendatahub-io/opendatahub-operator/pull/959/files#r1557861353 then this is deleteResource

There is name conflict here :)

Ah right, but then maybe the original one could be renamed to deleteResourceWithRetry or sth like that?

Updated. Kept One in the name

Updated. Kept One in the name
Too fast, variable name probably better to rename as well

Ah right, but then maybe the original one could be renamed to deleteResourceWithRetry or sth like that?

Pushed already :) Let's keep it then for further refactoring.

bartoszmajsak · 2024-04-09T15:33:56Z

pkg/upgrade/upgrade.go

@@ -297,9 +336,63 @@ func CleanupExistingResource(ctx context.Context, cli client.Client, platform de
 		Kind:    "OdhApplication",
 	}
 	multiErr = multierror.Append(multiErr, removOdhApplicationsCR(ctx, cli, JupyterhubApp, "jupyterhub", dscApplicationsNamespace))
+
+	// to take a reference
+	toDelete := jobGetDashboardWatsonResources(dscApplicationsNamespace)


if https://github.com/opendatahub-io/opendatahub-operator/pull/959/files#r1557861353 then remove job prefix here :)

Yes, makes sense. Thanks!

For the coming requirement to remove resources not just by their names but a field inside of spec, abstract the existing deleteDeprecatedResources/deleteDeprecatedServiceMonitors to take a resource to delete description as ``` type ResourceSpec struct { Gvk schema.GroupVersionKind Namespace string Path []string Values []string } ``` TODO: This patch just adds new functionality without changing the existing calls of deleteDeprecatedResources and deleteDeprecatedServiceMonitors yet. The function fetches UnstructuredList by gvk/namespace and uses public unstructured.NestedString() to access the field by path. It should work for ServiceMonitor, where ServiceMonitorList contains list of the pointers, as well. Ignore NoKindMatchError. CRD may not exist on fresh installation for example. Make a wrapper which takes an array of them for convenience and avoid one indentation level and simplifies multierror wrapping a bit. It requires to have a library of GroupVersionKinds which is a future work to make common for the whole project. Signed-off-by: Yauheni Kaliuta <[email protected]>

schema.GroupVersionKind objects are used around the code, introduce a common global place for it. Add Odh GVKs to be used in the next patch. Signed-off-by: Yauheni Kaliuta <[email protected]>

Jira: https://issues.redhat.com/browse/RHOAIENG-4764 Remove deprecated dashboard resources. Reuse existing infra just make the array of resources in a separate function to avoid growing of CleanupExistingResource(). Signed-off-by: Yauheni Kaliuta <[email protected]>

VaishnaviHire · 2024-04-10T15:07:02Z

/lgtm

openshift-ci · 2024-04-10T15:44:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: VaishnaviHire

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:

~~OWNERS~~ [VaishnaviHire]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…io#959) * pkg/upgrade: abstract resource deletion with Unstructured For the coming requirement to remove resources not just by their names but a field inside of spec, abstract the existing deleteDeprecatedResources/deleteDeprecatedServiceMonitors to take a resource to delete description as ``` type ResourceSpec struct { Gvk schema.GroupVersionKind Namespace string Path []string Values []string } ``` TODO: This patch just adds new functionality without changing the existing calls of deleteDeprecatedResources and deleteDeprecatedServiceMonitors yet. The function fetches UnstructuredList by gvk/namespace and uses public unstructured.NestedString() to access the field by path. It should work for ServiceMonitor, where ServiceMonitorList contains list of the pointers, as well. Ignore NoKindMatchError. CRD may not exist on fresh installation for example. Make a wrapper which takes an array of them for convenience and avoid one indentation level and simplifies multierror wrapping a bit. It requires to have a library of GroupVersionKinds which is a future work to make common for the whole project. Signed-off-by: Yauheni Kaliuta <[email protected]> * pkg/gvk: add package to store GroupVersionKind objects schema.GroupVersionKind objects are used around the code, introduce a common global place for it. Add Odh GVKs to be used in the next patch. Signed-off-by: Yauheni Kaliuta <[email protected]> * pkg/upgrade: remove watson-studio dashboard application Jira: https://issues.redhat.com/browse/RHOAIENG-4764 Remove deprecated dashboard resources. Reuse existing infra just make the array of resources in a separate function to avoid growing of CleanupExistingResource(). Signed-off-by: Yauheni Kaliuta <[email protected]> --------- Signed-off-by: Yauheni Kaliuta <[email protected]> (cherry picked from commit 23280e7)

pkg/upgrade: remove watson-studio dashboard application (#959)

…-services#959) * pkg/upgrade: abstract resource deletion with Unstructured For the coming requirement to remove resources not just by their names but a field inside of spec, abstract the existing deleteDeprecatedResources/deleteDeprecatedServiceMonitors to take a resource to delete description as ``` type ResourceSpec struct { Gvk schema.GroupVersionKind Namespace string Path []string Values []string } ``` TODO: This patch just adds new functionality without changing the existing calls of deleteDeprecatedResources and deleteDeprecatedServiceMonitors yet. The function fetches UnstructuredList by gvk/namespace and uses public unstructured.NestedString() to access the field by path. It should work for ServiceMonitor, where ServiceMonitorList contains list of the pointers, as well. Ignore NoKindMatchError. CRD may not exist on fresh installation for example. Make a wrapper which takes an array of them for convenience and avoid one indentation level and simplifies multierror wrapping a bit. It requires to have a library of GroupVersionKinds which is a future work to make common for the whole project. Signed-off-by: Yauheni Kaliuta <[email protected]> * pkg/gvk: add package to store GroupVersionKind objects schema.GroupVersionKind objects are used around the code, introduce a common global place for it. Add Odh GVKs to be used in the next patch. Signed-off-by: Yauheni Kaliuta <[email protected]> * pkg/upgrade: remove watson-studio dashboard application Jira: https://issues.redhat.com/browse/RHOAIENG-4764 Remove deprecated dashboard resources. Reuse existing infra just make the array of resources in a separate function to avoid growing of CleanupExistingResource(). Signed-off-by: Yauheni Kaliuta <[email protected]> --------- Signed-off-by: Yauheni Kaliuta <[email protected]>

openshift-ci bot added the do-not-merge/work-in-progress label Apr 8, 2024

ykaliuta force-pushed the delete-watson branch from 066f6e5 to 5bb54af Compare April 8, 2024 17:11

ykaliuta mentioned this pull request Apr 9, 2024

WIP: pkg/upgrade: abstract resource deletion with Action #957

Draft

3 tasks

ykaliuta force-pushed the delete-watson branch from 5bb54af to 3136cde Compare April 9, 2024 12:24

ykaliuta marked this pull request as ready for review April 9, 2024 12:25

openshift-ci bot removed the do-not-merge/work-in-progress label Apr 9, 2024

openshift-ci bot requested review from atheo89 and HumairAK April 9, 2024 12:25

zdtsw requested review from zdtsw and VaishnaviHire and removed request for HumairAK and atheo89 April 9, 2024 13:05

zdtsw reviewed Apr 9, 2024

View reviewed changes

bartoszmajsak reviewed Apr 9, 2024

View reviewed changes

ykaliuta force-pushed the delete-watson branch from 3136cde to 8f35446 Compare April 9, 2024 16:15

ykaliuta added 3 commits April 9, 2024 19:21

pkg/gvk: add package to store GroupVersionKind objects

ed4ad42

schema.GroupVersionKind objects are used around the code, introduce a common global place for it. Add Odh GVKs to be used in the next patch. Signed-off-by: Yauheni Kaliuta <[email protected]>

ykaliuta force-pushed the delete-watson branch from 8f35446 to b447013 Compare April 9, 2024 16:23

openshift-ci bot assigned VaishnaviHire Apr 10, 2024

openshift-ci bot added the lgtm label Apr 10, 2024

VaishnaviHire approved these changes Apr 10, 2024

View reviewed changes

openshift-ci bot added the approved label Apr 10, 2024

VaishnaviHire merged commit 23280e7 into opendatahub-io:incubation Apr 10, 2024
3 of 8 checks passed

VaishnaviHire referenced this pull request in red-hat-data-services/rhods-operator Apr 10, 2024

Merge pull request #242 from VaishnaviHire/sync_upstream_4_10

bde70ab

pkg/upgrade: remove watson-studio dashboard application (#959)

VaishnaviHire referenced this pull request in red-hat-data-services/rhods-operator Apr 10, 2024

Merge pull request #241 from VaishnaviHire/sync_upstream_4_10

1ca4aca

pkg/upgrade: remove watson-studio dashboard application (#959)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/upgrade: remove watson-studio dashboard application #959

pkg/upgrade: remove watson-studio dashboard application #959

ykaliuta commented Apr 8, 2024 •

edited

Loading

openshift-ci bot commented Apr 8, 2024

ykaliuta commented Apr 8, 2024

zdtsw Apr 9, 2024

zdtsw Apr 9, 2024

ykaliuta Apr 9, 2024

ykaliuta Apr 9, 2024 •

edited

Loading

zdtsw Apr 9, 2024

ykaliuta Apr 9, 2024

zdtsw commented Apr 9, 2024

bartoszmajsak Apr 9, 2024

bartoszmajsak Apr 9, 2024

ykaliuta Apr 9, 2024

bartoszmajsak Apr 9, 2024

ykaliuta Apr 9, 2024

ykaliuta Apr 9, 2024

ykaliuta Apr 9, 2024

bartoszmajsak Apr 9, 2024

ykaliuta Apr 9, 2024

VaishnaviHire commented Apr 10, 2024

openshift-ci bot commented Apr 10, 2024

	pathName := []string{"metadata", "name"}
	metaDataName := []string{"metadata", "name"}
	specAppName := []string{"spec", "appName"}

pkg/upgrade: remove watson-studio dashboard application #959

pkg/upgrade: remove watson-studio dashboard application #959

Conversation

ykaliuta commented Apr 8, 2024 • edited Loading

Description

How Has This Been Tested?

Merge criteria:

openshift-ci bot commented Apr 8, 2024

ykaliuta commented Apr 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ykaliuta Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zdtsw commented Apr 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VaishnaviHire commented Apr 10, 2024

openshift-ci bot commented Apr 10, 2024

ykaliuta commented Apr 8, 2024 •

edited

Loading

ykaliuta Apr 9, 2024 •

edited

Loading