use deployed MR for python e2e tests

[isinyaaa]

Fixes: #380

Description

Python e2e tests will use an environment variable to MR, delegating clean-up to a separate script.
The script is made environment-aware using envvars.

The CI has been updated to account for the changes, separating the Python workflows into 3 types to make it clearer.

How Has This Been Tested?

Merge criteria:

• All the commits have been signed-off (To pass the DCO check)

• The commits have meaningful messages; the author will squash them after approval or in case of manual merges will ask to merge with squash.
• 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.
• Code changes follow the kubeflow contribution guidelines.

If you have UI changes

• The developer has added tests or explained why testing cannot be added.
• Included any necessary screenshots or gifs if it was a UI change.
• Verify that UI/UX changes conform the UX guidelines for Kubeflow.

[tarilabs]

On brief look I like a lot the re-use of the kind startup part, the workflow-dispatch to trigger manually if desired and the industrialization of #380

thank you so far @isinyaaa @Al-Pragliola this is looking great already :)

[tarilabs]

as mentioned in #447 (comment) I really like this, thank you so much

Overall

/lgtm

with some comments below:

[tarilabs]

on a totally unrelated note, I would use analogous "injection approach" also for the other capability about #421

[isinyaaa]

I would prefer to use the serialization capabilities already available on Pydantic with the addition of Python-dotenv, would you prefer to have this change applied to this PR or later?

[tarilabs]

definitely for later.

I'm not opinionated on a library for bringing in the config values a-la Vyper or SmallRye Config, but we will need to support env variables, .env, and ConfigMaps, etc a-la 12Factor apps imho
@Al-Pragliola thoughts?

[Al-Pragliola]

Yes, we definitely need to support them as this approach makes everything more flexible 👍🏼

[Al-Pragliola]

what about using lsof -i :8080 -t to get the PID of a port-forward to 8080, if found we skip creating a new one? @isinyaaa @tarilabs

overall LGTM

[tarilabs]

to double-check.

during release, a PR with the updated manifest kustomization image setting is applied.
So first, the manifests are deployed to kind with an image tag which will not be found (the tag does not exists yet in the container registry).

This line (36) actually set the deployment model registry image to the one locally built, hence existing and loaded in the kind cluster.

Can this be confirmed, please?

[isinyaaa]

the image is being loaded on local mode inside the script. and on the CI it's loaded with a different command, but by the same method. So two cases here:

1. LOCAL envvar set:
   create the cluster and load the pre-built image inside, then run line 36 eventually.
2. No envvar set:
   assume the cluster already exists and has the image loaded. then we only set it after deploying MR (this avoids using kustomize directly which would change the manifests)

does this answer? otherwise @Al-Pragliola might be able to answer better as he suggested this change :)

[tarilabs]

thanks @isinyaaa I see, in case of GHA we do not set the LOCAL and we invoke directly the pytest via nox (instead of make):

https://github.com/kubeflow/model-registry/pull/447/files#diff-207f09dfa0bc604a684631a1c78be7d9aa00f9a6c778eec30afa287990be3da1R119-R143

that should do it.

I will wait for confirmation from @Al-Pragliola and likely will add some comments into it.

(no need to reply over weekend 😅 but I wanted to mark it down before forgetting 🙃 thanks a lot)

[Al-Pragliola]

I confirm what @isinyaaa just said, the job of the deploy_on_kind script is to first fill all the requirements for deploying mr and then deploy it on the current's context cluster.

In case of CI all requirements:

• existing Kubernetes cluster
• mr docker image built
• mr docker image present on the cluster's registry

are already done in the previous steps of the action, so we can just create the namespace and deploy by not setting the LOCAL env.

We decided to replace kustomize edit with kubectl edit for two reasons:

1. remove a dependency
2. avoid creating drifts, since kustomize actually modifies the manifests, which is a bit of a pain when running the script locally because the user has to undo local changes.

Hope this works well for everyone.

[Al-Pragliola]

LGTM

[tarilabs]

retested

/lgtm

[tarilabs]

/approve

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Al-Pragliola, tarilabs

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 [tarilabs]

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