Skip to content
This repository has been archived by the owner on Jan 31, 2024. It is now read-only.

Removing Data Science Pipelines' manifests #737

Merged

Conversation

DharmitD
Copy link
Member

@DharmitD DharmitD commented Feb 22, 2023

Description

Removing Data Science Pipelines' manifests from odh-manifests repository, the manifests will be managed by the new Data Science Pipelines Operator.
Keeping the Data Science Pipelines' standalone (operator-less) manifests in the Data Science Pipelines' (upstream & downstream) repositories.

Closes RHODS-6927

How Has This Been Tested?

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

@HumairAK
Copy link
Contributor

HumairAK commented Mar 1, 2023

This lgtm.

However this issue is blocked until #729 is merged. Once that's done this will need to be rebaesd.

@LaVLaS
Copy link
Member

LaVLaS commented Mar 8, 2023

@DharmitD Although we are replacing the existing data-science-pipelines deployment with deployments managed by the dsp operator in #729, we need to make sure that any users that have an existing kfdef referencing the current data-science-pipelines manifests are not blocked by an error in the odh-operator due to referencing a kustomizeConfig that no longer exists. We'll only need to maintain this for one or two versions while we tell users to migrate to the dsp-operator manifest

You will need to ensure that the manifests and overlays will "no-op" for anyone that may have a kfdef with the existing data-science-pipelines kustomizeConfig below

  - kustomizeConfig:
      overlays:
        - metadata-store-mariadb
        - ds-pipeline-ui
        - object-store-minio
        - default-configs
      repoRef:
        name: manifests
        path: data-science-pipelines
    name: data-science-pipelines

We did the same thing for odh-dashboard when we deprecated odh-dashboard/overlays/authentication

@LaVLaS
Copy link
Member

LaVLaS commented Mar 27, 2023

/hold

Putting a hold on this until the 1.5 release branch is tagged

@openshift-ci openshift-ci bot added the do-not-merge/hold Hold off on merging (provide reason in comment) label Mar 27, 2023
@LaVLaS
Copy link
Member

LaVLaS commented Apr 10, 2023

/unhold

1.5 release branch has been created, this can merge without any issues

@openshift-ci openshift-ci bot removed the do-not-merge/hold Hold off on merging (provide reason in comment) label Apr 10, 2023
@LaVLaS LaVLaS changed the title WIP: Removing Data Science Pipelines' manifests Removing Data Science Pipelines' manifests Apr 20, 2023
@DharmitD DharmitD marked this pull request as ready for review April 21, 2023 14:05
@DharmitD
Copy link
Member Author

@LaVLaS @anishasthana @VaishnaviHire this PR is now ready to be merged, thanks.

@LaVLaS LaVLaS self-requested a review April 21, 2023 18:53
LaVLaS
LaVLaS previously requested changes Apr 21, 2023
Copy link
Member

@LaVLaS LaVLaS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DharmitD You need need to remove data-science-pipelines from the tests kfdef so that CI tests will still work when this is merged.

@DharmitD
Copy link
Member Author

@DharmitD You need need to remove data-science-pipelines from the tests kfdef so that CI tests will still work when this is merged.

Done, updated to have data-science-pipelines removed.

@DharmitD
Copy link
Member Author

/retest

@VaishnaviHire
Copy link
Member

/test odh-manifests-e2e

@VaishnaviHire
Copy link
Member

@DharmitD This branch needs to be rebased with master

@LaVLaS LaVLaS self-requested a review April 25, 2023 13:17
@LaVLaS LaVLaS dismissed their stale review April 25, 2023 13:17

The test kfdef was updated to remove data-science-pipelines

@DharmitD
Copy link
Member Author

@DharmitD This branch needs to be rebased with master

Done, rebased it with master.

@openshift-ci
Copy link

openshift-ci bot commented Apr 25, 2023

@DharmitD: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/49-images 8a08e3b link true /test 49-images

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/test-infra repository. I understand the commands that are listed here.

@VaishnaviHire
Copy link
Member

/test 411-odh-manifests-e2e

@LaVLaS
Copy link
Member

LaVLaS commented Apr 25, 2023

@VaishnaviHire Assuming there are no majors issues directly related to the removal of data-science-pipelines then I'll merge this if the 4.11 test fails

@VaishnaviHire
Copy link
Member

This looks good. The tests for dashboard passed for both test clusters.

/lgtm

@openshift-ci
Copy link

openshift-ci bot commented Apr 25, 2023

[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:

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

@openshift-merge-robot openshift-merge-robot merged commit 31e25d4 into opendatahub-io:master Apr 25, 2023
6 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants