Skip to content
This repository has been archived by the owner on Oct 22, 2021. It is now read-only.

Add yaml linting #161

Merged
merged 6 commits into from
Nov 26, 2019
Merged

Add yaml linting #161

merged 6 commits into from
Nov 26, 2019

Conversation

viccuad
Copy link
Member

@viccuad viccuad commented Nov 18, 2019

Description

This PR adds yaml linting to the dev/ scripts and gitlab, which lints for yaml files, excluding deploy/helm/kubecf/*, as those are helm templates that should be better linted with helm lint.

To achieve it, it adds a bazel target //dev/linters:yamllint to install the yamllint python module, and a python wrapper to call it. I haven't found any other way in Bazel to pull a python module and then call it.

It provides the script dev/linters/yamllint.sh that calls the python wrapper. And it updates .gitlab-ci.yaml to call this last script.

Bazel's pip_install() doesn't install pip, so this PR needs f0rmiga/bazel-docker-image#3.

Motivation and Context

How Has This Been Tested?

I have tested it locally by running dev/linters/yamllint.sh.

I have tested it against gitlab-ci by editing the .gitlab-ci.yaml to zypper in pip, and running gitlab-runner exec docker lint.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code has security implications.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@viccuad
Copy link
Member Author

viccuad commented Nov 18, 2019

Linting the helm templates fail, eg:

./deploy/helm/kubecf/assets/operations/instance_groups/bits.yaml
  1:3       error    syntax error: expected the node content, but found '-'

We can't use helm lint as it only accepts Chart.yml as input. Another option would be to litter all templates with # yamllint disable-line, but that doesn't seem like a better option.

@viccuad viccuad added the Status: Blocked Dependencies on other issues and/or pull requests label Nov 18, 2019
@f0rmiga
Copy link
Member

f0rmiga commented Nov 19, 2019

#165 Should unblock this PR.

@fargozhu fargozhu added Type: CI Issues for working on the project's CI pipelines and removed Status: Blocked Dependencies on other issues and/or pull requests Status: WIP labels Nov 19, 2019
@viccuad
Copy link
Member Author

viccuad commented Nov 19, 2019

Rebased on top of master to include #165.
Edited PR message to signify that this PR lints yaml files excluding deploy/helm/kubecf/*, as those are helm templates that should be better linted with helm lint

@viccuad viccuad mentioned this pull request Nov 19, 2019
3 tasks
Copy link
Member

@f0rmiga f0rmiga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! This is the way to go with Python + Bazel!

WORKSPACE Outdated Show resolved Hide resolved
WORKSPACE Outdated Show resolved Hide resolved
dev/linters/BUILD.bazel Outdated Show resolved Hide resolved
dev/linters/BUILD.bazel Outdated Show resolved Hide resolved
dev/linters/BUILD.bazel Outdated Show resolved Hide resolved
dev/linters/run_yamllint.py Outdated Show resolved Hide resolved
doc/Contribute.md Outdated Show resolved Hide resolved
doc/Contribute.md Outdated Show resolved Hide resolved
dev/linters/yamllint.sh Outdated Show resolved Hide resolved
@f0rmiga f0rmiga removed the request for review from andreas-kupries November 19, 2019 20:05
dev/linters/yamllint.sh Outdated Show resolved Hide resolved
dev/linters/BUILD.bazel Outdated Show resolved Hide resolved
@viccuad
Copy link
Member Author

viccuad commented Nov 26, 2019

Many thanks for the review :). Also, I'm quite embarrassed by the simple mistakes I did.

I have fixed the typos and force-pushed with new changes, which I think it's gonna be cleaner:

  • Using git_repository instead of http_archive, needed for python3 support, saving the commit sha in def.bzl
  • pip_repositories and pip_import need to be evaluated at WORKSPACE-time, added commit comments. The rest can happily go on ./dev/linters/BUILD.bazel.

Python 3 compatibility is yet unreleased, pin commit for now.
Pip3 install yamllint by performing a 2-step process, first declare a pip
repostory with pip_repositories(), and then install the module and dependencies
with pip_install(). Pip needs to be executed at WORKSPACE-evaluation time.

Call yamllint by wrapping it on `dev/linters/run_yamllint.py`, using
py_binary() bazel function.
Use relaxed rules for only errors, don't check ./deploy/helm/kubecf/* as it
contains helm templates, but check ./deploy/helm/kubecf/values.* .
@viccuad
Copy link
Member Author

viccuad commented Nov 26, 2019

Rebased on top of master to fix conflicts.

Copy link
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes!

dev/linters/yamllint.sh Show resolved Hide resolved
@f0rmiga f0rmiga merged commit e194b81 into master Nov 26, 2019
@f0rmiga f0rmiga deleted the viccuad/add-yaml-linting branch November 26, 2019 22:02
bikramnehra pushed a commit that referenced this pull request Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: CI Issues for working on the project's CI pipelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants