Add test suite for tekton-kueue CEL expressions #7561

gbenhaim · 2025-08-10T12:46:03Z

Add hack/test-tekton-kueue-config.py with parameterized unittest
Test all CEL mutation scenarios: multi-platform, priority assignment, queue config
Dynamically read image from kustomization and config from development env
Add GitHub action to run tests on kueue-related PRs

Validates tekton-kueue mutations against expected annotations, labels, and priority classes using actual container via podman.

Also, update the tekton-kueue config on dev to match staging.

Assisted-By: Cursor

- Add hack/test-tekton-kueue-config.py with parameterized unittest - Test all CEL mutation scenarios: multi-platform, priority assignment, queue config - Dynamically read image from kustomization and config from development env - Add GitHub action to run tests on kueue-related PRs Validates tekton-kueue mutations against expected annotations, labels, and priority classes using actual container via podman. Also, update the tekton-kueue config on dev to match staging. Assisted-By: Cursor Signed-off-by: Gal Ben Haim <[email protected]>

github-actions · 2025-08-10T12:46:45Z

Code Review by Gemini

## Review of Changes

The changes introduce a comprehensive test suite for Tekton-Kueue CEL expressions, a new GitHub Actions workflow to run these tests, and a minor update to the `tekton-kueue` configuration. The overall approach is excellent, providing robust validation for the mutation logic.

### `components/kueue/development/tekton-kueue/config.yaml`

**Improvement:**

The change to include `replace(..., "_", "-")` is a good improvement for normalizing platform names, ensuring consistency in annotation keys.

### `hack/test-tekton-kueue-config.py`

**Potential Issue/Improvement:**

The `get_tekton_kueue_image` function could be made more robust to handle different `kustomization.yaml` structures for image overrides. Currently, it strictly requires both `newName` and `newTag` to be present. If `newName` is omitted (meaning the original `name` should be used), the current logic would fail.

**Suggested Change:**

```diff
--- a/hack/test-tekton-kueue-config.py
+++ b/hack/test-tekton-kueue-config.py
@@ -29,16 +29,19 @@
         
         # Look for the tekton-kueue image in the images section
         images = kustomization.get('images', [])
         for image in images:
             if image.get('name') == 'konflux-ci/tekton-kueue':
-                new_name = image.get('newName', '')
-                new_tag = image.get('newTag', '')
-                if new_name and new_tag:
-                    return f"{new_name}:{new_tag}"
+                # Use 'name' as fallback for 'newName' if 'newName' is not specified
+                image_name = image.get('newName', image.get('name'))
+                image_tag = image.get('newTag')
+                
+                if image_name and image_tag:
+                    return f"{image_name}:{image_tag}"
+                else:
+                    raise ValueError(f"Incomplete tekton-kueue image definition found in kustomization. Expected 'newTag' and optionally 'newName' for '{image.get('name')}'.")
         
-        raise ValueError("tekton-kueue image not found in kustomization")
+        raise ValueError("tekton-kueue image entry not found in kustomization for 'konflux-ci/tekton-kueue'")
     
     except Exception as e:
         raise RuntimeError(f"Failed to read tekton-kueue image from {KUSTOMIZATION_FILE}: {e}")

`.github/workflows/test-tekton-kueue-config.yaml`

Minor Improvement (Optional):

The "Report success" step is largely cosmetic as the job status already indicates success. It can be removed to slightly reduce workflow verbosity, but it's harmless to keep.

No specific line changes are strictly necessary for this file, as its current state is functional and clear.

Overall Assessment:

The changes are well-structured and significantly improve the test coverage for Tekton-Kueue CEL expressions. The use of podman to run the actual container for mutations is a robust approach. The new GitHub Actions workflow ensures these tests are run automatically, catching issues early. The hack/test-tekton-kueue-config.py script is well-documented and uses good practices like unittest and tempfile.

openshift-ci · 2025-08-12T06:44:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gbenhaim, mshaposhnik

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~~ [gbenhaim]

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

gbenhaim · 2025-08-12T09:20:48Z

/test appstudio-e2e-tests

filariow · 2025-08-12T09:32:28Z

hack/test-tekton-kueue-config.py

+CONFIG_FILE = REPO_ROOT / "components/kueue/development/tekton-kueue/config.yaml"
+KUSTOMIZATION_FILE = REPO_ROOT / "components/kueue/staging/base/tekton-kueue/kustomization.yaml"


We can rely on pathlib to inject the correct path separator for the current OS

Suggested change

CONFIG_FILE = REPO_ROOT / "components/kueue/development/tekton-kueue/config.yaml"

KUSTOMIZATION_FILE = REPO_ROOT / "components/kueue/staging/base/tekton-kueue/kustomization.yaml"

CONFIG_FILE = REPO_ROOT / "components" / "kueue" / "development" / "tekton-kueue" / "config.yaml"

KUSTOMIZATION_FILE = REPO_ROOT / "components" / "kueue" / "staging" / "base" / "tekton-kueue" / "kustomization.yaml"```

filariow · 2025-08-12T09:43:26Z

hack/test-tekton-kueue-config.py

+        print("Checking prerequisites...")
+
+        # Check config file
+        if CONFIG_FILE.exists():
+            print(f"✓ Config file found: {CONFIG_FILE}")
+        else:
+            print(f"✗ Config file not found: {CONFIG_FILE}")
+            sys.exit(1)
+
+        # Check kustomization file
+        if KUSTOMIZATION_FILE.exists():
+            print(f"✓ Kustomization file found: {KUSTOMIZATION_FILE}")
+        else:
+            print(f"✗ Kustomization file not found: {KUSTOMIZATION_FILE}")
+            sys.exit(1)
+
+        # Check tekton-kueue image
+        try:
+            image = get_tekton_kueue_image()
+            print(f"✓ Tekton-kueue image: {image}")
+        except Exception as e:
+            print(f"✗ Failed to get tekton-kueue image: {e}")
+            sys.exit(1)
+
+        # Check podman
+        try:
+            result = subprocess.run(["podman", "--version"], capture_output=True, check=True, text=True)
+            print(f"✓ Podman available: {result.stdout.strip()}")
+        except (subprocess.CalledProcessError, FileNotFoundError):
+            print("✗ Podman is not available")
+            sys.exit(1)
+
+        print("\n✅ All prerequisites met! Ready to run tests.")
+        print("Run: python hack/test-tekton-kueue-config.py")
+        print("\nNote: Tests will FAIL (not skip) if any prerequisites are missing.")


minor: for readability, I'd suggest moving this into a dedicated function

def checkPrerequisites(): # ...

Suggested change

print("Checking prerequisites...")

# Check config file

if CONFIG_FILE.exists():

print(f"✓ Config file found: {CONFIG_FILE}")

else:

print(f"✗ Config file not found: {CONFIG_FILE}")

sys.exit(1)

# Check kustomization file

if KUSTOMIZATION_FILE.exists():

print(f"✓ Kustomization file found: {KUSTOMIZATION_FILE}")

else:

print(f"✗ Kustomization file not found: {KUSTOMIZATION_FILE}")

sys.exit(1)

# Check tekton-kueue image

try:

image = get_tekton_kueue_image()

print(f"✓ Tekton-kueue image: {image}")

except Exception as e:

print(f"✗ Failed to get tekton-kueue image: {e}")

sys.exit(1)

# Check podman

try:

result = subprocess.run(["podman", "--version"], capture_output=True, check=True, text=True)

print(f"✓ Podman available: {result.stdout.strip()}")

except (subprocess.CalledProcessError, FileNotFoundError):

print("✗ Podman is not available")

sys.exit(1)

print("\n✅ All prerequisites met! Ready to run tests.")

print("Run: python hack/test-tekton-kueue-config.py")

print("\nNote: Tests will FAIL (not skip) if any prerequisites are missing.")

checkPrerequisites()

filariow · 2025-08-12T09:50:16Z

hack/test-tekton-kueue-config.py

+        """Set up test class - check prerequisites."""
+        # Check if config file exists
+        if not CONFIG_FILE.exists():
+            raise FileNotFoundError(f"Config file not found: {CONFIG_FILE}")
+
+        # Check if kustomization file exists
+        if not KUSTOMIZATION_FILE.exists():
+            raise FileNotFoundError(f"Kustomization file not found: {KUSTOMIZATION_FILE}")
+
+        # Get the tekton-kueue image from kustomization
+        try:
+            cls.tekton_kueue_image = get_tekton_kueue_image()
+            print(f"Using tekton-kueue image: {cls.tekton_kueue_image}")
+        except Exception as e:
+            raise RuntimeError(f"Failed to get tekton-kueue image: {e}")
+
+        # Check if podman is available
+        try:
+            subprocess.run(["podman", "--version"], capture_output=True, check=True)
+        except FileNotFoundError:
+            raise RuntimeError("podman command not found. Please install podman.")
+        except subprocess.CalledProcessError as e:
+            raise RuntimeError(f"podman is not working properly: {e}")


it seems to me this logic is a duplicate of the check prerequisite block in main. We could move it to a dedicated function and call it from here too.

Also, as this logic is executed as part of unit tests too, does it make sense to suggest in documentation at L12 to call python hack/test-tekton-kueue-config.py --check-setup before python hack/test-tekton-kueue-config.py?

filariow · 2025-08-12T10:05:46Z

hack/test-tekton-kueue-config.py

+            # Check annotations
+            annotations = mutated.get("metadata", {}).get("annotations", {})
+            for expected_annotation in expected["annotations"]:
+                self.assertIn(expected_annotation, annotations, 
+                             f"Expected annotation {expected_annotation} not found")
+                self.assertEqual(annotations[expected_annotation], "1",
+                               f"Expected annotation {expected_annotation} to have value '1'")
+
+            # Check labels
+            labels = mutated.get("metadata", {}).get("labels", {})
+            for expected_label, expected_value in expected["labels"].items():
+                self.assertIn(expected_label, labels,
+                             f"Expected label {expected_label} not found")
+                self.assertEqual(labels[expected_label], expected_value,
+                               f"Expected label {expected_label} to have value '{expected_value}', got '{labels.get(expected_label)}'")


I'd suggest checking the whole result of the mutation with assertDictEqual

Suggested change

# Check annotations

annotations = mutated.get("metadata", {}).get("annotations", {})

for expected_annotation in expected["annotations"]:

self.assertIn(expected_annotation, annotations,

f"Expected annotation {expected_annotation} not found")

self.assertEqual(annotations[expected_annotation], "1",

f"Expected annotation {expected_annotation} to have value '1'")

# Check labels

labels = mutated.get("metadata", {}).get("labels", {})

for expected_label, expected_value in expected["labels"].items():

self.assertIn(expected_label, labels,

f"Expected label {expected_label} not found")

self.assertEqual(labels[expected_label], expected_value,

f"Expected label {expected_label} to have value '{expected_value}', got '{labels.get(expected_label)}'")

self.assertDictEqual(mutated, expected)

This will require some changes in the test data too.

I tested with the following changes: git diff. Expand this block, that's collapsed for better readability.

diff --git a/hack/test-tekton-kueue-config.py b/hack/test-tekton-kueue-config.py index 02d7be8f..0e736fbe 100755 --- a/hack/test-tekton-kueue-config.py +++ b/hack/test-tekton-kueue-config.py @@ -116,15 +116,37 @@ TEST_PIPELINERUNS = { } }, "expected": { - "annotations": [ - "kueue.konflux-ci.dev/requests-linux-amd64", - "kueue.konflux-ci.dev/requests-linux-arm64", - "kueue.konflux-ci.dev/requests-linux-s390x" - ], - "labels": { - "kueue.x-k8s.io/queue-name": "pipelines-queue", - "kueue.x-k8s.io/priority-class": "konflux-post-merge-build" - } + "apiVersion": "tekton.dev/v1", + "kind": "PipelineRun", + "metadata": { + "name": "test-multiplatform-new", + "namespace": "default", + "creationTimestamp": None, + "annotations": { + "kueue.konflux-ci.dev/requests-linux-amd64": "1", + "kueue.konflux-ci.dev/requests-linux-arm64": "1", + "kueue.konflux-ci.dev/requests-linux-s390x": "1", + }, + "labels": { + "pipelinesascode.tekton.dev/event-type": "push", + "kueue.x-k8s.io/queue-name": "pipelines-queue", + "kueue.x-k8s.io/priority-class": "konflux-post-merge-build", + } + }, + "spec": { + "status": "PipelineRunPending", + "taskRunTemplate": {}, + "pipelineRef": {"name": "build-pipeline"}, + "params": [ + { + "name": "build-platforms", + "value": ["linux/amd64", "linux/arm64", "linux/s390x"] + }, + {"name": "other-param", "value": "test"} + ], + "workspaces": [{"name": "shared-workspace", "emptyDir": {}}] + }, + "status": {}, } }, @@ -149,7 +171,7 @@ TEST_PIPELINERUNS = { "taskRef": {"name": "build-task"} }, { - "name": "build-task-arm64", + "name": "build-task-arm64", "params": [{"name": "PLATFORM", "value": "linux/arm64"}], "taskRef": {"name": "build-task"} }, @@ -163,14 +185,46 @@ TEST_PIPELINERUNS = { } }, "expected": { - "annotations": [ - "kueue.konflux-ci.dev/requests-linux-amd64", - "kueue.konflux-ci.dev/requests-linux-arm64" - ], - "labels": { - "kueue.x-k8s.io/queue-name": "pipelines-queue", - "kueue.x-k8s.io/priority-class": "konflux-pre-merge-build" - } + "apiVersion": "tekton.dev/v1", + "kind": "PipelineRun", + "metadata": { + "name": "test-multiplatform-old", + "namespace": "default", + "creationTimestamp": None, + "annotations": { + "kueue.konflux-ci.dev/requests-linux-amd64": "1", + "kueue.konflux-ci.dev/requests-linux-arm64": "1", + }, + "labels": { + "pipelinesascode.tekton.dev/event-type": "pull_request", + "kueue.x-k8s.io/queue-name": "pipelines-queue", + "kueue.x-k8s.io/priority-class": "konflux-pre-merge-build" + }, + }, + "spec": { + "status": "PipelineRunPending", + "taskRunTemplate": {}, + "pipelineSpec": { + "tasks": [ + { + "name": "build-task-amd64", + "params": [{"name": "PLATFORM", "value": "linux/amd64"}], + "taskRef": {"name": "build-task"} + }, + { + "name": "build-task-arm64", + "params": [{"name": "PLATFORM", "value": "linux/arm64"}], + "taskRef": {"name": "build-task"} + }, + { + "name": "other-task", + "taskRef": {"name": "other-task"} + } + ] + }, + "workspaces": [{"name": "shared-workspace", "emptyDir": {}}] + }, + "status": {}, } }, @@ -193,11 +247,26 @@ TEST_PIPELINERUNS = { } }, "expected": { - "annotations": [], - "labels": { - "kueue.x-k8s.io/queue-name": "pipelines-queue", - "kueue.x-k8s.io/priority-class": "konflux-release" - } + "apiVersion": "tekton.dev/v1", + "kind": "PipelineRun", + "metadata": { + "name": "test-release-managed", + "namespace": "default", + "creationTimestamp": None, + "labels": { + "kueue.x-k8s.io/queue-name": "pipelines-queue", + "kueue.x-k8s.io/priority-class": "konflux-release", + "appstudio.openshift.io/service": "release", + "pipelines.appstudio.openshift.io/type": "managed" + } + }, + "spec": { + "status": "PipelineRunPending", + "taskRunTemplate": {}, + "pipelineRef": {"name": "release-pipeline"}, + "workspaces": [{"name": "shared-workspace", "emptyDir": {}}] + }, + "status": {}, } }, @@ -220,11 +289,26 @@ TEST_PIPELINERUNS = { } }, "expected": { - "annotations": [], - "labels": { - "kueue.x-k8s.io/queue-name": "pipelines-queue", - "kueue.x-k8s.io/priority-class": "konflux-tenant-release" - } + "apiVersion": "tekton.dev/v1", + "kind": "PipelineRun", + "metadata": { + "name": "test-release-tenant", + "namespace": "default", + "creationTimestamp": None, + "labels": { + "appstudio.openshift.io/service": "release", + "pipelines.appstudio.openshift.io/type": "tenant", + "kueue.x-k8s.io/queue-name": "pipelines-queue", + "kueue.x-k8s.io/priority-class": "konflux-tenant-release", + } + }, + "spec": { + "status": "PipelineRunPending", + "taskRunTemplate": {}, + "pipelineRef": {"name": "release-pipeline"}, + "workspaces": [{"name": "shared-workspace", "emptyDir": {}}] + }, + "status": {}, } }, @@ -243,11 +327,24 @@ TEST_PIPELINERUNS = { } }, "expected": { - "annotations": [], - "labels": { - "kueue.x-k8s.io/queue-name": "pipelines-queue", - "kueue.x-k8s.io/priority-class": "konflux-dependency-update" - } + "apiVersion": "tekton.dev/v1", + "kind": "PipelineRun", + "metadata": { + "name": "test-mintmaker", + "namespace": "mintmaker", + "creationTimestamp": None, + "labels": { + "kueue.x-k8s.io/queue-name": "pipelines-queue", + "kueue.x-k8s.io/priority-class": "konflux-dependency-update", + } + }, + "spec": { + "status": "PipelineRunPending", + "taskRunTemplate": {}, + "pipelineRef": {"name": "dependency-update-pipeline"}, + "workspaces": [{"name": "shared-workspace", "emptyDir": {}}] + }, + "status": {}, } }, @@ -269,11 +366,25 @@ TEST_PIPELINERUNS = { } }, "expected": { - "annotations": [], - "labels": { - "kueue.x-k8s.io/queue-name": "pipelines-queue", - "kueue.x-k8s.io/priority-class": "konflux-post-merge-test" - } + "apiVersion": "tekton.dev/v1", + "kind": "PipelineRun", + "metadata": { + "name": "test-integration-test-push", + "namespace": "default", + "creationTimestamp": None, + "labels": { + "kueue.x-k8s.io/queue-name": "pipelines-queue", + "kueue.x-k8s.io/priority-class": "konflux-post-merge-test", + "pac.test.appstudio.openshift.io/event-type": "push", + } + }, + "spec": { + "status": "PipelineRunPending", + "taskRunTemplate": {}, + "pipelineRef": {"name": "integration-test-pipeline"}, + "workspaces": [{"name": "shared-workspace", "emptyDir": {}}] + }, + "status": {}, } }, @@ -295,11 +406,25 @@ TEST_PIPELINERUNS = { } }, "expected": { - "annotations": [], - "labels": { - "kueue.x-k8s.io/queue-name": "pipelines-queue", - "kueue.x-k8s.io/priority-class": "konflux-pre-merge-test" - } + "apiVersion": "tekton.dev/v1", + "kind": "PipelineRun", + "metadata": { + "name": "test-integration-test-pr", + "namespace": "default", + "creationTimestamp": None, + "labels": { + "pac.test.appstudio.openshift.io/event-type": "pull_request", + "kueue.x-k8s.io/queue-name": "pipelines-queue", + "kueue.x-k8s.io/priority-class": "konflux-pre-merge-test", + } + }, + "spec": { + "status": "PipelineRunPending", + "taskRunTemplate": {}, + "pipelineRef": {"name": "integration-test-pipeline"}, + "workspaces": [{"name": "shared-workspace", "emptyDir": {}}] + }, + "status": {}, } }, @@ -315,14 +440,27 @@ TEST_PIPELINERUNS = { "spec": { "pipelineRef": {"name": "default-pipeline"}, "workspaces": [{"name": "shared-workspace", "emptyDir": {}}] - } + }, }, "expected": { - "annotations": [], - "labels": { - "kueue.x-k8s.io/queue-name": "pipelines-queue", - "kueue.x-k8s.io/priority-class": "konflux-default" - } + "apiVersion": "tekton.dev/v1", + "kind": "PipelineRun", + "metadata": { + "name": "test-default", + "namespace": "default", + "creationTimestamp": None, + "labels": { + "kueue.x-k8s.io/queue-name": "pipelines-queue", + "kueue.x-k8s.io/priority-class": "konflux-default" + }, + }, + "spec": { + "status": "PipelineRunPending", + "taskRunTemplate": {}, + "pipelineRef": {"name": "default-pipeline"}, + "workspaces": [{"name": "shared-workspace", "emptyDir": {}}] + }, + "status": {}, } } } @@ -408,21 +546,7 @@ class TektonKueueMutationTest(unittest.TestCase): mutated = self.run_mutation_test(test_data) expected = test_data["expected"] - # Check annotations - annotations = mutated.get("metadata", {}).get("annotations", {}) - for expected_annotation in expected["annotations"]: - self.assertIn(expected_annotation, annotations, - f"Expected annotation {expected_annotation} not found") - self.assertEqual(annotations[expected_annotation], "1", - f"Expected annotation {expected_annotation} to have value '1'") - - # Check labels - labels = mutated.get("metadata", {}).get("labels", {}) - for expected_label, expected_value in expected["labels"].items(): - self.assertIn(expected_label, labels, - f"Expected label {expected_label} not found") - self.assertEqual(labels[expected_label], expected_value, - f"Expected label {expected_label} to have value '{expected_value}', got '{labels.get(expected_label)}'") + self.assertDictEqual(mutated, expected) def test_all_mutations(self): """Test all tekton-kueue mutation scenarios."""

gbenhaim · 2025-08-12T11:35:53Z

/hold

scoheb · 2025-08-12T18:57:16Z

/test appstudio-e2e-tests

scoheb · 2025-08-12T20:15:54Z

/test appstudio-e2e-tests

Dannyb48 · 2025-08-12T22:01:18Z

/test appstudio-e2e-tests

openshift-ci bot requested review from ralphbean and skabashnyuk August 10, 2025 12:46

openshift-ci bot added the approved label Aug 10, 2025

gbenhaim requested review from mshaposhnik, sadlerap, manish-jangra, eisraeli and enkeefe00 August 10, 2025 12:46

mshaposhnik approved these changes Aug 12, 2025

View reviewed changes

mshaposhnik added the lgtm label Aug 12, 2025

filariow reviewed Aug 12, 2025

View reviewed changes

openshift-ci bot added the do-not-merge/hold label Aug 12, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add test suite for tekton-kueue CEL expressions #7561

Add test suite for tekton-kueue CEL expressions #7561

gbenhaim commented Aug 10, 2025

Uh oh!

github-actions bot commented Aug 10, 2025

Uh oh!

openshift-ci bot commented Aug 12, 2025

Uh oh!

gbenhaim commented Aug 12, 2025

Uh oh!

filariow Aug 12, 2025

Uh oh!

filariow Aug 12, 2025

Uh oh!

filariow Aug 12, 2025

Uh oh!

filariow Aug 12, 2025 •

edited

Loading

Uh oh!

gbenhaim commented Aug 12, 2025

Uh oh!

scoheb commented Aug 12, 2025

Uh oh!

scoheb commented Aug 12, 2025

Uh oh!

Dannyb48 commented Aug 12, 2025

Uh oh!

Uh oh!

		CONFIG_FILE = REPO_ROOT / "components/kueue/development/tekton-kueue/config.yaml"
		KUSTOMIZATION_FILE = REPO_ROOT / "components/kueue/staging/base/tekton-kueue/kustomization.yaml"

Add test suite for tekton-kueue CEL expressions #7561

Are you sure you want to change the base?

Add test suite for tekton-kueue CEL expressions #7561

Conversation

gbenhaim commented Aug 10, 2025

Uh oh!

github-actions bot commented Aug 10, 2025

Code Review by Gemini

.github/workflows/test-tekton-kueue-config.yaml

Uh oh!

openshift-ci bot commented Aug 12, 2025

Uh oh!

gbenhaim commented Aug 12, 2025

Uh oh!

filariow Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

filariow Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

filariow Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

filariow Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gbenhaim commented Aug 12, 2025

Uh oh!

scoheb commented Aug 12, 2025

Uh oh!

scoheb commented Aug 12, 2025

Uh oh!

Dannyb48 commented Aug 12, 2025

Uh oh!

Uh oh!

`.github/workflows/test-tekton-kueue-config.yaml`

filariow Aug 12, 2025 •

edited

Loading