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

Replace old model-mesh manifests to new manifests #815

Merged
merged 4 commits into from
May 30, 2023

Conversation

Jooho
Copy link
Contributor

@Jooho Jooho commented May 15, 2023

Description

The old model mesh manifest was independent of the upstream model mesh, so when the upstream model mesh manifest changed, it was very difficult to update the opendatahub manifest to follow its history. So the new manifest uses the upstream manifest as much as possible via an overlay, with only minor changes.
Inside this folder, there are now two main component manifest folders: odh-model-controller, odh-modelmesh-controller
Each component utilized the manifest from the main repository as much as possible and used overlay or params.env for other components.

How Has This Been Tested?

The way how to use the model-mesh manifests is the same as before so you can test this with the kfdef that we used.

kind: KfDef
apiVersion: kfdef.apps.kubeflow.org/v1
metadata:
  name: opendatahub
  namespace: opendatahub
spec:
  applications:
    - kustomizeConfig:
        repoRef:
          name: manifests
          path: odh-common
      name: odh-common
    - kustomizeConfig:
        repoRef:
          name: manifests
          path: model-mesh
      name: model-mesh
  repos:
    - name: manifests
      uri: 'https://github.com/Jooho/odh-manifests/tarball/new-mm-manifests'

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

ISSUE: https://issues.redhat.com/browse/RHODS-8375

@Jooho Jooho force-pushed the new-mm-manifests branch 3 times, most recently from 1d28852 to e6259e6 Compare May 16, 2023 10:34
@anishasthana
Copy link
Member

/retest

@Jooho
Copy link
Contributor Author

Jooho commented May 18, 2023

/retest

1 similar comment
@anishasthana
Copy link
Member

/retest

@anishasthana
Copy link
Member

/retest

@anishasthana
Copy link
Member

/retest-required

1 similar comment
@anishasthana
Copy link
Member

/retest-required

@Jooho
Copy link
Contributor Author

Jooho commented May 23, 2023

/retest

@Jooho
Copy link
Contributor Author

Jooho commented May 23, 2023

@anishasthana Finally this passed all tests!. Could you please review this PR?

model-mesh/base/params.env Show resolved Hide resolved
model-mesh/OWNERS Show resolved Hide resolved
@danielezonca
Copy link

/lgtm

Well done :)

Copy link
Member

@anishasthana anishasthana left a comment

Choose a reason for hiding this comment

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

Approving as two component owners have approved/lgtm'd it.

@openshift-ci
Copy link

openshift-ci bot commented May 30, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anishasthana, danielezonca

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 78c3d5c into opendatahub-io:master May 30, 2023
@shalberd
Copy link
Contributor

Follow-up minor issue relared to serving runtimes: https://github.com/opendatahub-io/odh-manifests/issues/809

Inconsistent usage of kustomization.images refs vs parameter (ovms).

@shalberd
Copy link
Contributor

shalberd commented Jun 1, 2023

another error that came up previous to this master-level change, fixed with the changes in this PR / master:

svens-mbp-2:odh-manifests-1.6.1 s$ pwd
/Users/s/Downloads/odh-manifests-1.6.1

svens-mbp-2:odh-manifests-1.6.1 s$ kustomize version

{Version:kustomize/v4.5.6 GitCommit:29ca6935bde25565795e1b4e13ca211c4aa56417 BuildDate:2022-07-29T20:42:23Z GoOs:darwin GoArch:amd64}

svens-mbp-2:odh-manifests-1.6.1 s$ kustomize build  ./model-mesh/base
Error: accumulating resources: accumulation err='accumulating resources from '../dependencies/quickstart.yaml': security; file '/Users/s/Downloads/odh-manifests-1.6.1/model-mesh/dependencies/quickstart.yaml' is not in or below '/Users/s/Downloads/odh-manifests-1.6.1/model-mesh/base'': must build at directory: '/Users/s/Downloads/odh-manifests-1.6.1/model-mesh/dependencies/quickstart.yaml': file is not directory

@Jooho Jooho mentioned this pull request Jun 1, 2023
3 tasks
Jooho added a commit to Jooho/odh-manifests that referenced this pull request Jun 14, 2023
Jooho added a commit to red-hat-data-services/odh-manifests that referenced this pull request Jun 16, 2023
[UPSTREAM] Replace old model-mesh manifests to new manifests opendatahub-io#815
@@ -39,7 +50,7 @@ Following are the steps to install Model Mesh as a part of OpenDataHub install:

1. Install the OpenDataHub operator
2. Create a KfDef that includes the model-mesh component with the odh-model-controller overlay.
Copy link

@bdattoma bdattoma Jul 4, 2023

Choose a reason for hiding this comment

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

with the odh-model-controller overlay. should this text be removed since the overlay got removed? @Jooho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good Catcher. Right, this should be deleted.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants