Closed Bug 1414824 Opened 7 years ago Closed 7 years ago

Enable stricter linting in taskcluster yaml files.

Categories

(Firefox Build System :: Task Configuration, task)

Product:
Firefox Build System
Component:
Task Configuration
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla59

People

(Reporter: Callek, Assigned: stevea1, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [lang=python])

Attachments

(1 file, 1 obsolete file)

Bug 1414824 - Enable stricter linting in taskcluster yaml files.
59 bytes, text/x-review-board-request
Callek
: review+
Details 
We currently disable a bunch of linting in taskcluster yaml files. We should enable more.

https://yamllint.readthedocs.io/en/latest/

https://dxr.mozilla.org/mozilla-central/source/taskcluster/.yamllint
I'm open to mentoring this bug.

What I require:

* A unique commit/patch for each yaml linting rule enabled/fixed.  (This eases review and clarity.)

How To:

* Clone mozilla central  (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code/Mercurial)
* Run the linter (`./mach lint -l yaml taskcluster`)
  - You should see no errors
* edit the taskcluster/.yamllint file to enable a currently disabled lint check.
* Re-run the linter
* Edit files with issues
* Commit the change to mercurial using this bug number (e.g. `hg commit -m "Bug 1414824 - Lint more taskcluster yaml, enable brackets r?Callek"
* Repeat for other linting errors
* Push to mozreview (http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview.html)

I can be found in either #taskcluster or #releng on IRC should you have any questions.

First person to post a patch (even if just a partial set) will get assigned this bug.
Mentor: bugspam.Callek
Keywords: good-first-bug
It was suggested to me to specify lang=python (even though this is yaml) to help bubble up this bugs visibility.
Whiteboard: [lang=python]
I've enabled checks for all but line-length. I can tackle that next if this looks good.
Assignee: nobody → stevea1
Comment on attachment 8927538 [details]
Bug 1414824 - Enable stricter linting in taskcluster yaml files.

https://reviewboard.mozilla.org/r/198846/#review204222

All in all this looks pretty good. Thank you a lot for the patch.

I have one issue with the removal of document-start headers, and then I think we're pretty good.

Additionally I would love if you could rebase this ontop of mozilla-central, it had some merge conflicts when I tried to rebase it myself to see if it was going to be good for landing. (If you need help figuring out how to do so, you can reach out to me here or in IRC).

Lastly, I'm not as worried about the line-length, but we can do that in a different bug if you think it would improve readability I'm open to getting that fixed too.

::: taskcluster/.yamllint
(Diff revision 1)
>    # Checks currently failing
> -  brackets: disable
> -  commas: disable
> -  comments: disable
> -  comments-indentation: disable
> -  document-start: disable

Lets leave `document-start: disable` here (and not force it as missing) see the comment on docker_linux_loaner.yml

::: taskcluster/taskgraph/actions/docker_linux_loaner.yml:19
(Diff revision 1)
>      * Set the environment variable TASKCLUSTER_INTERACTIVE=true
>  order: 1
>  context:
>    - worker-implementation: docker-worker
>      os: linux
> ----
> +

I think removing these document start tags has a potential to break stuff. Lets revert the document-start: missing bit
Attachment #8927538 - Flags: review?(bugspam.Callek)
I rebased on the latest from mozilla-central so I think that should be all set. I disabled the document-start check too.

Looking through the instances of long lines, I see quite a few that are long path names. I'm not sure wrangling them under a certain character limit would improve readability, so I'd say leave that check out for now or until someone who feels more strongly wants to tackle it!
Comment on attachment 8927538 [details]
Bug 1414824 - Enable stricter linting in taskcluster yaml files.

https://reviewboard.mozilla.org/r/198846/#review204530

Thank you for the rebase, this applied cleanly, however has some rebase errors. (changes that are not just linting related). Can you please fix those up and we can get this landed.

Thank you for the quick turnaround here.

::: taskcluster/ci/push-apk-breakpoint/kind.yml
(Diff revision 2)
>          attributes:
>              build_platform: android-nightly
>              nightly: true
> -        shipping-phase: ship
> +        worker-type:  # see transforms
> -        shipping-product: fennec
> -        worker-type: # see transforms

Bad rebase... we need shipping-phase and shipping-product not to be deleted.

::: taskcluster/ci/release-bouncer-aliases/kind.yml:60
(Diff revision 2)
> -               maple:
> +                maple:
> -                  - "release-drivers-staging"
> +                    - "release-drivers-staging"
> -               try:
> +                try:
> -                  #- "{task[tags][createdForUser]}"
> +                #   - "{task[tags][createdForUser]}"
> -               default:
> +                default:
> -                  - "release-drivers"
> +                    - "release-drivers"

This file also has a some rebase issues

::: taskcluster/ci/release-uptake-monitoring/kind.yml:1
(Diff revision 2)
>  # This Source Code Form is subject to the terms of the Mozilla Public

more rebase issues here...

::: taskcluster/taskgraph/actions/docker_linux_loaner.yml:19
(Diff revision 2)
>      * Set the environment variable TASKCLUSTER_INTERACTIVE=true
>  order: 1
>  context:
>    - worker-implementation: docker-worker
>      os: linux
> ----
> +

need to keep these document start markers

::: taskcluster/taskgraph/templates/artifact.yml
(Diff revision 2)
> ----

keep this document start marker please

::: taskcluster/taskgraph/templates/rebuild.yml
(Diff revision 2)
> ----

need to keep these document start markers
Attachment #8927538 - Flags: review?(bugspam.Callek)
Comment on attachment 8927538 [details]
Bug 1414824 - Enable stricter linting in taskcluster yaml files.

https://reviewboard.mozilla.org/r/198846/#review204530

> Bad rebase... we need shipping-phase and shipping-product not to be deleted.

Wow, clearly I don't have this rebase stuff down quite yet - sorry about that. I believe I've got this and the other goofs corrected now.
Comment on attachment 8927538 [details]
Bug 1414824 - Enable stricter linting in taskcluster yaml files.

https://reviewboard.mozilla.org/r/198846/#review205032

Very very close this time, thank you again.  Just one thing I noticed.

::: taskcluster/ci/test/compiled.yml
(Diff revision 3)
>          chunked:
> -          by-test-platform:
> +            by-test-platform:
> -            windows.*: false
> +                windows.*: false
> -            macosx.*: false
> +                macosx.*: false
> -            default: true
> +                default: true
> -    when:

looks like an accidental removal
Attachment #8927538 - Flags: review?(bugspam.Callek)
Comment on attachment 8927538 [details]
Bug 1414824 - Enable stricter linting in taskcluster yaml files.

https://reviewboard.mozilla.org/r/198846/#review205032

> looks like an accidental removal

Ok, fixed that up.
Attachment #8927538 - Attachment is obsolete: true
Attachment #8927538 - Flags: review?(bugspam.Callek)
Steve, thank you again for the patch, I went ahead and fixed up the last of the rebase issues and rebased for the final time against central.

There were not many issues, but I figured the back and fourth of the rebasing was eating up more of your time than I'd have expected.

To validate the rebase, since these changes were nearly all whitespace only, I did `hg diff -r central::. -w` which allowed me to see a diff without whitespace. Once I corrected the remaining things I re-pushed to mozreview and then rebased and pushed again.

I'm about to schedule this patch to autoland, and it has proper attribution for you, since the work is still yours.

I'm not certain how you went about rebasing, my method was `hg rebase -r . -d central` and using a configured merge tool of "kdiff3"  (though in my rebase I didn't have any merge conflicts)

Thanks again for this patch!
Comment on attachment 8929844 [details]
Bug 1414824 - Enable stricter linting in taskcluster yaml files.

https://reviewboard.mozilla.org/r/201044/#review206212
Attachment #8929844 - Flags: review?(bugspam.Callek) → review+
Pushed by Callek@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/237e09de43a1
Enable stricter linting in taskcluster yaml files. r=Callek
https://hg.mozilla.org/mozilla-central/rev/237e09de43a1
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: