Closed Bug 1337360 Opened 3 years ago Closed 3 years ago

Enforce use of dashes in YAML schemas

Categories

(Firefox Build System :: Task Configuration, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla55

People

(Reporter: dustin, Assigned: dustin)

Details

Attachments

(3 files, 1 obsolete file)

It's easier to use YAML if you know that identifiers look-like-this and not_like_this muchLessLikeThis.

Let's enforce that, and adjust the few uses of interCaps and underscores to suit.
Comment on attachment 8834549 [details]
Bug 1337360: enforce use of dashes in YAML schemas;

https://reviewboard.mozilla.org/r/110434/#review112062

::: taskcluster/taskgraph/transforms/balrog.py:87
(Diff revision 1)
>  
>          label = job.get('label', "balrog-{}".format(dep_job.label))
>  
>          upstream_artifacts = [{
> -            "taskId": {"task-reference": "<beetmover>"},
> -            "taskType": "beetmover",
> +            "task-id": {"task-reference": "<beetmover>"},
> +            "task-type": "beetmover",

I'm not really a fan of this conversion, I understand why the overall yaml should use dashes, but modifying transform specificity in terms of what is *actually* required outside of the transforms makes source traceability harder.

e.g. I search for taskId and I find few/one use, and need to trace 'task-id' after that.

I'll need to think on it, or be convinced a bit more.
That's a pretty convincing argument :)

One option may be to whitelist certain fields like upstreamArtifacts, with a comment on the whitelist indicating that `-` is preferred, but it's OK to use camel case or underscores for cases where the data structure is handed to an external service.

I'll sit on this for a while and think about it.
Attachment #8834549 - Flags: review?(bugspam.Callek) → review-
Attachment #8834549 - Attachment is obsolete: true
Comment on attachment 8850121 [details]
Bug 1337360: check for schema elements that aren't dashed-identifiers, with whitelist;

https://reviewboard.mozilla.org/r/122816/#review125480

::: taskcluster/taskgraph/util/schema.py:181
(Diff revision 1)
> +    Operates identically to voluptuous.Schema, but applying some taskgraph-specific checks
> +    in the process.
> +    """
> +    schema = voluptuous.Schema(*args, **kwargs)
> +    check_schema(schema)
> +    return schema

My only concern here is someone using voluptuous.Schema instead of this Schema function. I'm not sure of any good way to rectify though...
Attachment #8850121 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8850122 [details]
Bug 1337360: move chainOfTrust into extra;

https://reviewboard.mozilla.org/r/122818/#review125482
Attachment #8850122 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8850123 [details]
Bug 1337360: remove unnecessary schema for attributes;

https://reviewboard.mozilla.org/r/122820/#review125486
Attachment #8850123 - Flags: review?(bugspam.Callek) → review+
Pushed by dmitchell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cd66ec07e1a1
check for schema elements that aren't dashed-identifiers, with whitelist; r=Callek
https://hg.mozilla.org/integration/autoland/rev/14874597101b
move chainOfTrust into extra; r=Callek
https://hg.mozilla.org/integration/autoland/rev/12ba77d6b7d1
remove unnecessary schema for attributes; r=Callek
https://hg.mozilla.org/mozilla-central/rev/cd66ec07e1a1
https://hg.mozilla.org/mozilla-central/rev/14874597101b
https://hg.mozilla.org/mozilla-central/rev/12ba77d6b7d1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.