Closed Bug 1387862 Opened 2 years ago Closed 2 years ago

We should have CI Lint YAML files in the tree

Categories

(Firefox Build System :: Lint and Formatting, enhancement, P2)

3 Branch
enhancement

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: Callek, Assigned: Callek)

Details

Attachments

(7 files)

We have a lot of .yml files in our tree these days, we should try and be consistent with them, and avoid things like duplicate-keys everywhere.
Comment on attachment 8894244 [details]
Bug 1387862 - Add initial support for ./mach lint -l yaml

https://reviewboard.mozilla.org/r/165324/#review170808
Attachment #8894244 - Flags: review?(dustin) → review+
Comment on attachment 8894246 [details]
Bug 1387862 - Lint taskcluster's cron.yml file, fixup lint errors.

https://reviewboard.mozilla.org/r/165328/#review170810
Attachment #8894246 - Flags: review?(dustin) → review+
Comment on attachment 8894248 [details]
Bug 1387862 - Lint NSS's taskcluster.yml.

https://reviewboard.mozilla.org/r/165332/#review170812
Attachment #8894248 - Flags: review?(dustin) → review+
Comment on attachment 8894245 [details]
Bug 1387862 - Lint taskcluster yaml files.

https://reviewboard.mozilla.org/r/165326/#review170818
Attachment #8894245 - Flags: review?(dustin) → review+
Comment on attachment 8894250 [details]
Bug 1387862 - Add yaml lint to the taskgraph.

https://reviewboard.mozilla.org/r/165336/#review170814
Attachment #8894250 - Flags: review?(dustin) → review+
Comment on attachment 8894244 [details]
Bug 1387862 - Add initial support for ./mach lint -l yaml

https://reviewboard.mozilla.org/r/165324/#review171206

Awesome, this is 90% good. I think the config file hack I did for flake8 should not be copied here though.

::: tools/lint/yamllint_/__init__.py:33
(Diff revision 1)
>  
>  results = []
>  
>  
> -class Flake8Process(ProcessHandlerMixin):
> +def normalize_to_topsrcdir(path):
> +    topsrcdir = os.path.normpath(os.path.join(here, '..', '..', '..'))

topsrcdir should be in lintargs['root'], please grab it from there instead (should also be in YAMLLintProcess.config).

::: tools/lint/yamllint_/__init__.py:148
(Diff revision 1)
> -    # Run any paths with a .flake8 file in the directory separately so
> -    # it gets picked up. This means only .flake8 files that live in
> +    # Run any paths with a .yamllint file in the directory separately so
> +    # it gets picked up. This means only .yamllint files that live in
>      # directories that are explicitly included will be considered.
> -    # See bug 1277851
>      no_config = []
>      for f in files:
> -        if not os.path.isfile(os.path.join(f, '.flake8')):
> +        yamllint_config = os.path.join(f, '.yamllint')
> +        if not os.path.isfile(yamllint_config):
>              no_config.append(f)
>              continue
> -        run_process(config, cmdargs+[f])
> +        run_process(config,
> +                    gen_yamllint_args(cmdargs, conf_file=yamllint_config, paths=f))
>  
> -    # XXX For some reason passing in --exclude results in flake8 not using
> +    # Only support excludes in the no-config mode for now
> -    # the local .flake8 file. So for now only pass in --exclude if there
> -    # is no local config.
>      exclude = lintargs.get('exclude')
> -    if exclude:
> -        cmdargs += ['--exclude', ','.join(lintargs['exclude'])]
>  
>      if no_config:
> -        run_process(config, cmdargs+no_config)
> +        run_process(config,
> +                    gen_yamllint_args(cmdargs, excludes=exclude, paths=no_config))

This whole section was to work around limitations in flake8. It's actually a pretty bad hack and I've been wanting to fix it for some time now.

I don't know how config files in yamllint work (or even if there are config files).. but I'd recommend not copying it and only calling `run_process` from one place. Even if it means not supporting multiple config to start (we can work out how to add multi-config support in the future if it's really needed).

::: tools/lint/yamllint.yml:3
(Diff revision 1)
> +yamllint:
> +    description: YAML linter
> +    extensions: ['yml']

Should probably add `yaml` here too just in case.
Attachment #8894244 - Flags: review?(ahalberstadt) → review-
Comment on attachment 8894247 [details]
Bug 1387862 - Lint python/mozlint yaml files.

https://reviewboard.mozilla.org/r/165330/#review171212
Attachment #8894247 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8894250 [details]
Bug 1387862 - Add yaml lint to the taskgraph.

https://reviewboard.mozilla.org/r/165336/#review171214

::: taskcluster/ci/source-test/mozlint.yml:79
(Diff revision 1)
> +    when:
> +        files-changed:
> +            - '**/*.yml'
> +            - '**/.ymllint'
> +            - 'python/mozlint/**'
> +            - 'taskcluster/ci/**'

Is this one necessary? Relevant changes here should be picked up by `**/*.yml`. Also, should add `**/*.yaml` here too.
Attachment #8894250 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8894249 [details]
Bug 1387862 - Lint the linter config yaml files themselves.

https://reviewboard.mozilla.org/r/165334/#review171216
Attachment #8894249 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8894244 [details]
Bug 1387862 - Add initial support for ./mach lint -l yaml

https://reviewboard.mozilla.org/r/165324/#review171206

> This whole section was to work around limitations in flake8. It's actually a pretty bad hack and I've been wanting to fix it for some time now.
> 
> I don't know how config files in yamllint work (or even if there are config files).. but I'd recommend not copying it and only calling `run_process` from one place. Even if it means not supporting multiple config to start (we can work out how to add multi-config support in the future if it's really needed).

With yamllint the config file support is actually a bit worse than pep8 aiui. Basically you pass either -c file or pass it as part of the -d json blob. And it doesn't even take a yamllint file from the directory your file path is in, it looks in cwd (and a few other magic locations) instead.

So if we want to support in-tree yamllint configs outside of "one true place" this is how to do it, and I think we should.

I'll ping you on IRC to discuss.
Comment on attachment 8894244 [details]
Bug 1387862 - Add initial support for ./mach lint -l yaml

https://reviewboard.mozilla.org/r/165324/#review171206

> topsrcdir should be in lintargs['root'], please grab it from there instead (should also be in YAMLLintProcess.config).

It wasn't in YAMLLintProcess.config, and I had to copy it to the config to get it into YAMLLintProcess properly, but fixed.

> With yamllint the config file support is actually a bit worse than pep8 aiui. Basically you pass either -c file or pass it as part of the -d json blob. And it doesn't even take a yamllint file from the directory your file path is in, it looks in cwd (and a few other magic locations) instead.
> 
> So if we want to support in-tree yamllint configs outside of "one true place" this is how to do it, and I think we should.
> 
> I'll ping you on IRC to discuss.

We discussed and it seems that the limitation in flake8 does apply to here. It will be best to get it upstream but until we can its ok for here. I'll file a followup bug on finding files in parent dir (and caching the searched dirs)

> Should probably add `yaml` here too just in case.

We also discussed removing --exclude magic. Which I did because yamllint itself doesn't support that arg, and trying to magically support it wasn't helpful here.
Comment on attachment 8894244 [details]
Bug 1387862 - Add initial support for ./mach lint -l yaml

https://reviewboard.mozilla.org/r/165324/#review171684

Thanks, lgtm!

::: commit-message-36ad8:1
(Diff revision 2)
> +Bug 1387862 - Add initial support for ./mach lint -l yamllint r=ahal r=dustin

Commit message needs updating, -l yaml
Attachment #8894244 - Flags: review?(ahalberstadt) → review+
Pushed by Callek@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cc4c01794114
Add initial support for ./mach lint -l yaml r=ahal,dustin
https://hg.mozilla.org/integration/autoland/rev/5bb5dc7655ec
Lint taskcluster yaml files. r=dustin
https://hg.mozilla.org/integration/autoland/rev/e0bfb35b0eec
Lint taskcluster's cron.yml file, fixup lint errors. r=dustin
https://hg.mozilla.org/integration/autoland/rev/22c1094e387f
Lint python/mozlint yaml files. r=ahal
https://hg.mozilla.org/integration/autoland/rev/3713ea9672e8
Lint NSS's taskcluster.yml. r=dustin
https://hg.mozilla.org/integration/autoland/rev/a85b7e7d9f24
Lint the linter config yaml files themselves. r=ahal
https://hg.mozilla.org/integration/autoland/rev/63f87f6db7d6
Add yaml lint to the taskgraph. r=ahal,dustin
Ryan backed out for me, because I didn't update the final patch (making taskgraph use -l yaml instead of -l yamllint) :(
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0cd4ced60476
Backed out 7 changesets for yaml linting failures.
Severity: normal → enhancement
Priority: -- → P2
Pushed by Callek@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/98c08b9cc52e
Add initial support for ./mach lint -l yaml r=ahal,dustin
https://hg.mozilla.org/integration/autoland/rev/a86a804e64ee
Lint taskcluster yaml files. r=dustin
https://hg.mozilla.org/integration/autoland/rev/c13bd0798514
Lint taskcluster's cron.yml file, fixup lint errors. r=dustin
https://hg.mozilla.org/integration/autoland/rev/103073e92350
Lint python/mozlint yaml files. r=ahal
https://hg.mozilla.org/integration/autoland/rev/9e1723c2e1c9
Lint NSS's taskcluster.yml. r=dustin
https://hg.mozilla.org/integration/autoland/rev/57176d7ff82a
Lint the linter config yaml files themselves. r=ahal
https://hg.mozilla.org/integration/autoland/rev/155c5392cac2
Add yaml lint to the taskgraph. r=ahal,dustin
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #45)
> TEST-UNEXPECTED-WARNING | .cron.yml:51:16 | too few spaces before comment
> (comments)

This was due to a change in .cron.yml only on autoland :(. Fixed
Flags: needinfo?(bugspam.Callek)
Pushed by Callek@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/86f77dbf3248
Add initial support for ./mach lint -l yaml r=ahal,dustin
https://hg.mozilla.org/integration/autoland/rev/15a578334064
Lint taskcluster yaml files. r=dustin
https://hg.mozilla.org/integration/autoland/rev/4d38db927cc9
Lint taskcluster's cron.yml file, fixup lint errors. r=dustin
https://hg.mozilla.org/integration/autoland/rev/fafce110b9f6
Lint python/mozlint yaml files. r=ahal
https://hg.mozilla.org/integration/autoland/rev/e6d7c04887d3
Lint NSS's taskcluster.yml. r=dustin
https://hg.mozilla.org/integration/autoland/rev/56bd4017fcf9
Lint the linter config yaml files themselves. r=ahal
https://hg.mozilla.org/integration/autoland/rev/ffa9e965a111
Add yaml lint to the taskgraph. r=ahal,dustin
Product: Testing → Firefox Build System
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.