1337360 - Enforce use of dashes in YAML schemas

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

[Dustin J. Mitchell [:dustin] (he/him)]

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 1

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: Bug 1337360: enforce use of dashes in YAML schemas;

Review commit: https://reviewboard.mozilla.org/r/110434/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110434/

Comment 2

[Justin Wood (:Callek)]

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.

Comment 3

[Dustin J. Mitchell [:dustin] (he/him)]

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.

Comment 4

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: Bug 1337360: check for schema elements that aren't dashed-identifiers, with whitelist;

Review commit: https://reviewboard.mozilla.org/r/122816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122816/

Comment 5

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: Bug 1337360: move chainOfTrust into extra;

Review commit: https://reviewboard.mozilla.org/r/122818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122818/

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: Bug 1337360: remove unnecessary schema for attributes;

Review commit: https://reviewboard.mozilla.org/r/122820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122820/

Comment 7

[Justin Wood (:Callek)]

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...

Comment 8

[Justin Wood (:Callek)]

Comment on attachment 8850122 [details]
Bug 1337360: move chainOfTrust into extra;

https://reviewboard.mozilla.org/r/122818/#review125482

Comment 9

[Justin Wood (:Callek)]

Comment on attachment 8850123 [details]
Bug 1337360: remove unnecessary schema for attributes;

https://reviewboard.mozilla.org/r/122820/#review125486

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cd66ec07e1a1
https://hg.mozilla.org/mozilla-central/rev/14874597101b
https://hg.mozilla.org/mozilla-central/rev/12ba77d6b7d1