refactor: get rid of hardcoded attribute filtering in find_tasks #225
Conversation
| @@ -25,8 +25,12 @@ def make_firefoxci_artifact_tasks(config, tasks): | |||
| for task in tasks: | |||
| # Format: { <task id>: [<artifact name>]} | |||
| tasks_to_create = defaultdict(list) | |||
| include_attrs = task.pop("include-attrs", {}) | |||
| exclude_attrs = task.pop("exclude-attrs", {}) | |||
There was a problem hiding this comment.
This can be done in a follow-up, but it might be nice to group include-attrs/exclude-attrs with individual index paths. E.g:
from-decision-indices:
- path: <index path>
include-attrs: [<attrs>]
exclude-attrs: [<attrs>]
This would allow a bit more flexibility.
|
|
||
| for attr, values in dict(exclude_attrs).items(): | ||
| if any([attributes.get(attr, "").startswith(v) for v in values]): | ||
| skip = True |
There was a problem hiding this comment.
This feels like it should be using attrmatch:
https://github.com/taskcluster/taskgraph/blob/main/src/taskgraph/util/attributes.py#L9
But I guess that wouldn't support the exclude case? It might be nice for attrmatch to support excluding attributes as well (maybe by prepending ! to the name or something).
But this is also follow-up territory.
There was a problem hiding this comment.
Actually you could call attrmatch twice. Once for include-attrs, and a second time for exclude-attrs but then negate the return value.
There was a problem hiding this comment.
TIL about attrmatch. Indeed, let's use that instead.
There was a problem hiding this comment.
Switching to this has highlighted that it also may be nice to support prefixes natively in attrmatch; for now I've wrapped that portion in a callable that attrmatch can use.
There was a problem hiding this comment.
Ah, I missed the startswith, good call
bhearsum
force-pushed
the
hardcoded-attrs
branch
from
94a7dd5 to
70a27fd
Compare
No description provided.