RHOAIENG-16055: new(tests): test to start a Workbench, by creating the Notebook CR directly #94

jiridanek · 2025-01-15T17:04:03Z

Description

This is a port of

https://github.com/skodjob/odh-e2e/blob/main/src/test/java/io/odh/test/e2e/standard/NotebookST.java#L78-L125

How Has This Been Tested?

figure out when/how to use unprivileged k8s client

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

github-actions · 2025-01-15T17:04:21Z

The following are automatically added/executed:

PR size label.
Run pre-commit
Run tox

Available user actions:

To mark a PR as WIP, add /wip in a comment. To remove it from the PR comment /wip cancel to the PR.
To block merging of a PR, add /hold in a comment. To un-block merging of PR comment /hold cancel.
To mark a PR as approved, add /lgtm in a comment. To remove, add /lgtm cancel.
lgtm label removed on each new commit push.
To mark PR as verified comment /verified to the PR, to un-verify comment /verified cancel to the PR.
verified label removed on each new commit push.

Supported labels

{'/hold', '/verified', '/lgtm', '/wip'}

for more information, see https://pre-commit.ci RHOAIENG-16055: new(tests): test to start a Workbench, by creating the Notebook CR directly

…e Notebook CR directly

for more information, see https://pre-commit.ci

rnetser · 2025-01-19T07:57:53Z

tests/workbenches/conftest.py

@@ -0,0 +1,309 @@
+from __future__ import annotations
+
+import logging


please use simple_logger

rnetser · 2025-01-19T08:00:37Z

tests/workbenches/conftest.py

+    resource_manager.destroy()
+
+
+class KubeResourceManager:


please use ocpenshift python wrapper to CRUD resources

rnetser · 2025-01-19T08:02:38Z

tests/workbenches/conftest.py

+    OPENSHIFT_DOMAIN = "openshift.io/"
+    ODH_DOMAIN = "opendatahub.io/"


used in other places as well, please place under https://github.com/opendatahub-io/opendatahub-tests/blob/main/utilities/constants.py and re-use

rnetser · 2025-01-19T08:03:04Z

tests/workbenches/conftest.py

+
+    LABEL_DASHBOARD = ODH_DOMAIN + "dashboard"
+    LABEL_ODH_MANAGED = ODH_DOMAIN + "odh-managed"
+    LABEL_SIDECAR_ISTIO_INJECT = "sidecar.istio.io/inject"


rnetser · 2025-01-19T08:04:13Z

tests/workbenches/conftest.py

+        self.privileged_client = privileged_client
+        self.resources: list[ocp_resources.resource.Resource] = []
+
+    def createResourceWithoutWait(self, client: DynamicClient, resource: ocp_resources.resource.Resource):


please use python naming conventions

rnetser · 2025-01-19T08:09:43Z

tests/workbenches/notebook-controller/test_spawning.py

+import kubernetes.client
+from kubernetes.dynamic import DynamicClient
+
+from tests.workbenches.conftest import OdhAnnotationsLabels, OdhConstants, PodUtils


under conftest place fixtures; functions and classes should be under utils.py in the relevant dir

rnetser · 2025-01-19T08:10:50Z

tests/workbenches/notebook-controller/test_spawning.py

+    @classmethod
+    @property
+    @functools.cache
+    def logger(cls):


please use simple_logger

rnetser · 2025-01-19T08:12:26Z

tests/workbenches/notebook-controller/test_spawning.py

+            ),
+        },
+    )
+    def testCreateSimpleNotebook(self, function_resource_manager, admin_client, unprivileged_client):


all setup steps (e.g namespace, pvc) should be done in fixtures
please check existing fixtures and re-use

rnetser · 2025-01-19T08:13:33Z

tests/workbenches/notebook-controller/test_spawning.py

+# }
+
+
+def loadDefaultNotebook(client: DynamicClient, namespace: str, name: str, image: str) -> Notebook:


functions should go under utils.py in the relevant dir

rnetser · 2025-01-19T08:18:32Z

tests/workbenches/notebook-controller/test_data/notebook.yaml

@@ -0,0 +1,146 @@
+apiVersion: kubeflow.org/v1
+kind: Notebook


is there a template for this in the cluster?

lugi0 · 2025-01-20T08:47:01Z

tests/workbenches/notebook-controller/test_spawning.py

+
+        with allure.step("Create Notebook CR"):
+            # notebookImage: str = NotebookType.getNotebookImage(NotebookType.JUPYTER_MINIMAL_IMAGE, NotebookType.JUPYTER_MINIMAL_2023_2_TAG);
+            notebookImage: str = "quay.io/modh/odh-minimal-notebook-container@sha256:615af25cfd4f3f2981b173e1a5ab24cb79f268ee72dabbddb6867ee1082eb902"


Can this be fetched dinamically from the cluster?

lugi0 · 2025-01-20T08:47:35Z

tests/workbenches/notebook-controller/test_spawning.py

+    notebookString = pathlib.Path(
+        "/Users/jdanek/IdeaProjects/opendatahub-tests/tests/workbenches/notebook-controller/test_data/notebook.yaml"
+    ).read_text()


Pretty sure this won't work :)

lugi0 · 2025-01-20T08:49:31Z

tests/workbenches/notebook-controller/test_spawning.py

+    afterTestSteps={
+        Step(
+            value="Delete ODH operator and all created resources",
+            expected="Operator is removed and all other resources as well",
+        )
+    },


I don't think this has been functionally implemented but FWIW we don't plan to remove the operator after the test run

lugi0 · 2025-01-20T08:52:58Z

tests/workbenches/docs.py

@@ -0,0 +1,39 @@
+from typing import Callable


What is this used for? does it just provide additional documentation for tests in e.g. https://github.com/opendatahub-io/opendatahub-tests/pull/94/files#diff-358f1122838e1ac22415822c5a85d419e2d111c64285e488695af1cf6f03d490R35 ?
IMHO if this is implemented we should use it across the repo to keep consistency, so this should go under a higher level directory, but it should probably be discussed beforehand. Can you attend the shift left wg meeting to do so?

lugi0 · 2025-01-20T08:54:07Z

tests/workbenches/conftest.py

@@ -0,0 +1,309 @@
+from __future__ import annotations


I think the majority of these classes could be avoided if you instead used the wrapper library https://github.com/RedHatQE/openshift-python-wrapper

jiridanek marked this pull request as draft January 15, 2025 17:04

github-actions bot added size/xl size/xxl and removed size/xl labels Jan 15, 2025

jiridanek marked this pull request as ready for review January 17, 2025 09:27

jiridanek added 4 commits January 17, 2025 10:29

[pre-commit.ci] auto fixes from pre-commit.com hooks

87c56c8

for more information, see https://pre-commit.ci RHOAIENG-16055: new(tests): test to start a Workbench, by creating the Notebook CR directly

RHOAIENG-16055: new(tests): test to start a Workbench, by creating th…

c696def

…e Notebook CR directly

fixup, implement everything so that test runs to completion

7199032

uv lock

92744bb

jiridanek force-pushed the jd_notebooks_first branch from 330e5e2 to 92744bb Compare January 17, 2025 09:30

github-actions bot added size/xl and removed size/xxl labels Jan 17, 2025

jiridanek added 3 commits January 17, 2025 10:37

fixup precommit secret detection

5a79b41

fixup precommit add types

67271a2

fixup precommit reformat

004d870

jiridanek force-pushed the jd_notebooks_first branch from 3f686b4 to 004d870 Compare January 17, 2025 09:43

fixup precommit types

c1eddd8

jiridanek force-pushed the jd_notebooks_first branch from f5edaa0 to c1eddd8 Compare January 17, 2025 09:49

[pre-commit.ci] auto fixes from pre-commit.com hooks

5d73d41

for more information, see https://pre-commit.ci

rnetser requested changes Jan 19, 2025

View reviewed changes

lugi0 reviewed Jan 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RHOAIENG-16055: new(tests): test to start a Workbench, by creating the Notebook CR directly #94

RHOAIENG-16055: new(tests): test to start a Workbench, by creating the Notebook CR directly #94

jiridanek commented Jan 15, 2025 •

edited

Loading

github-actions bot commented Jan 15, 2025

rnetser Jan 19, 2025

rnetser Jan 19, 2025

rnetser Jan 19, 2025

rnetser Jan 19, 2025

rnetser Jan 19, 2025

rnetser Jan 19, 2025

rnetser Jan 19, 2025

rnetser Jan 19, 2025

rnetser Jan 19, 2025

rnetser Jan 19, 2025

lugi0 Jan 20, 2025

lugi0 Jan 20, 2025

lugi0 Jan 20, 2025

lugi0 Jan 20, 2025

lugi0 Jan 20, 2025

		@@ -0,0 +1,309 @@
		from __future__ import annotations

		import logging

		OPENSHIFT_DOMAIN = "openshift.io/"
		ODH_DOMAIN = "opendatahub.io/"

		# }


		def loadDefaultNotebook(client: DynamicClient, namespace: str, name: str, image: str) -> Notebook:

RHOAIENG-16055: new(tests): test to start a Workbench, by creating the Notebook CR directly #94

Are you sure you want to change the base?

RHOAIENG-16055: new(tests): test to start a Workbench, by creating the Notebook CR directly #94

Conversation

jiridanek commented Jan 15, 2025 • edited Loading

Description

How Has This Been Tested?

Merge criteria:

github-actions bot commented Jan 15, 2025

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

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

jiridanek commented Jan 15, 2025 •

edited

Loading