Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/taskgraph/transforms/task.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,14 @@
from dataclasses import dataclass
from typing import Callable, Literal, Optional, Union

import voluptuous

from taskgraph.transforms.base import TransformSequence
from taskgraph.util.hash import hash_path
from taskgraph.util.keyed_by import evaluate_keyed_by
from taskgraph.util.schema import (
IndexSchema,
LegacySchema,
OptimizationType,
Schema,
TaskPriority,
Expand Down Expand Up @@ -194,6 +197,14 @@ class PayloadBuilder:


def payload_builder(name, schema):
if isinstance(schema, dict):
schema = LegacySchema(
{
voluptuous.Required("implementation"): name,
voluptuous.Optional("os"): str,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand why we're adding these fields for plain-dict schemas. Wouldn't the incoming schemas have them if they are required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was existing behavior, this is from the v18.1 of taskgraph : https://github.com/taskcluster/taskgraph/blob/18.1.0/src/taskgraph/transforms/task.py#L437 . We can eventually drop support for this once everything is converted over

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, thank you - I was unable to dig up that reference. sgtm

}
).extend(schema)

def wrap(func):
assert name not in payload_builders, f"duplicate payload builder name {name}"
payload_builders[name] = PayloadBuilder(schema, func)
Expand Down
3 changes: 3 additions & 0 deletions src/taskgraph/util/schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ def validate_schema(schema, obj, msg_prefix):
# Handle plain Python types (e.g. str, int) via msgspec.convert
elif isinstance(schema, type):
msgspec.convert(obj, schema)
# Handle plain dict schemas (e.g. from downstream payload builders)
elif isinstance(schema, dict):
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the plan for plain-dict schemas going forward? Will they be removed in favour of msgspec ones? Will this block need to be updated to convert them to msgspec schemas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eventually yes since all plain dict seems get convert to voluptuous. For cross compatibility we will still need to support them for the time being

voluptuous.Schema(schema)(obj)
else:
raise TypeError(f"Unsupported schema type: {type(schema)}")
except (
Expand Down
34 changes: 34 additions & 0 deletions test/test_transforms_task.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from pprint import pprint

import pytest
import voluptuous
from pytest_taskgraph import FakeParameters

from taskgraph.transforms import task
Expand Down Expand Up @@ -966,3 +967,36 @@ def test_task_priority(run_transform, graph_config, test_task):
assert task_dict["task"]["priority"] == priority
else:
assert task_dict["task"]["priority"] == graph_config["task-priority"]


@pytest.fixture
def dict_schema_builder():
@task.payload_builder("test-builder", schema={"command": [str]})
def _builder(config, task, task_def):
pass

yield task.payload_builders["test-builder"].schema
task.payload_builders.pop("test-builder", None)


@pytest.mark.parametrize(
"payload",
(
{"implementation": "test-builder", "command": ["echo"]},
{"implementation": "test-builder", "command": ["echo"], "os": "linux"},
),
)
def test_dict_schema_accepts_valid_payload(dict_schema_builder, payload):
dict_schema_builder(payload)


@pytest.mark.parametrize(
"payload",
(
{"implementation": "wrong-name", "command": ["echo"]},
{"command": ["echo"]},
),
)
def test_dict_schema_rejects_invalid_payload(dict_schema_builder, payload):
with pytest.raises(voluptuous.MultipleInvalid):
dict_schema_builder(payload)
12 changes: 12 additions & 0 deletions test/test_util_schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,18 @@ def test_index_schema_accepts_all_fields(self):
)


class TestValidateSchemaDictHandler(unittest.TestCase):
"""validate_schema must accept plain dict schemas passed
by downstream payload builders without raising TypeError."""

def test_dict_schema_valid(self):
validate_schema({"name": str, "count": int}, {"name": "a", "count": 1}, "pfx")

def test_dict_schema_invalid(self):
with self.assertRaises(Exception):
validate_schema({"name": str}, {"name": 123}, "pfx")


def test_optionally_keyed_by():
typ = optionally_keyed_by("foo", str, use_msgspec=True)
assert msgspec.convert("baz", typ) == "baz"
Expand Down
Loading