fix: restore payload_builder wrapping for dict schemas#910
fix: restore payload_builder wrapping for dict schemas#910abhishekmadan30 merged 1 commit intotaskcluster:mainfrom
Conversation
jcristau
left a comment
jcristau
left a comment
There was a problem hiding this comment.
Can we add test coverage for this?
Yep ! |
abhishekmadan30
force-pushed
the
payload-builder-fix
branch
4 times, most recently
from
698ca57 to
5659659
Compare
| elif isinstance(schema, type): | ||
| msgspec.convert(obj, schema) | ||
| # Handle plain dict schemas (e.g. from downstream payload builders) | ||
| elif isinstance(schema, dict): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| schema = LegacySchema( | ||
| { | ||
| voluptuous.Required("implementation"): name, | ||
| voluptuous.Optional("os"): str, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Ah, thank you - I was unable to dig up that reference. sgtm
abhishekmadan30
force-pushed
the
payload-builder-fix
branch
from
5659659 to
60d18eb
Compare
3 participants
No description provided.