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

Add yaml linter in github workflow #1410

Merged
merged 3 commits into from
Jan 7, 2022
Merged

Conversation

aman556
Copy link
Contributor

@aman556 aman556 commented Dec 19, 2021

What this PR does / why we need it

This PR is adding yaml linter in Github workflow.

Files added and changes in this PR.

  • A check-yaml.yaml file is added in .github/workflows which is adding yaml linter in github workflow.
  • Makefile under repo is changed to add a make target for yaml inter.
  • A bash script check-yaml.sh is added under hack dir. (the reason to add this bash script under hack is that other check-mdlint.sh and check-license.sh is there so following that pattern)
  • A config file is added which is required for yaml linter the location is hack/linter as cli-wordlist.yaml is there so assumed that that is the workspace to add the yaml files there.

check-yaml.sh working

  • first downloading a container with random name and then running that container
  • checking all the yaml files in tanzu-framework repo for the enable checks in the .yamllntconfig.yaml file.
  • If errors are there in the yaml files then the script is returning with exit code 1 and breaking the flow after printing the error and file name.

Which issue(s) this PR fixes

Fixes #1322

Describe testing done for PR

Ran make yamllint

Release note

yaml linter in github workflow

PR Checklist

  • Squash the commits into one or a small number of logical commits
  • Use good commit messages
  • Ensure PR contains terms all contributors can understand and links all contributors can access

Additional information

NA

Special notes for your reviewer

NA

@aman556 aman556 requested a review from a team as a code owner December 19, 2021 06:20
@aman556
Copy link
Contributor Author

aman556 commented Dec 19, 2021

Hii tanzu-framework team I want to know that are there any files which we want to ignore to pass from yaml linter so that i can ignore them and correct the error and warning in other files.

@vuil
Copy link
Contributor

vuil commented Jan 3, 2022

After looking at #1399, I wonder if this should folded into the lint target as well? We already have quite a handful of Lint-y checks as it is. cc @stmcginnis

@stmcginnis
Copy link
Contributor

I wonder if this should folded into the lint target as well?

I don't think I have a strong opinion about which way we go, but I do think we should either choose to have each type of linting we want to perform be separate, discrete GitHub Action checks, or we should have them all be a part of the one lint target. It's a bit confusing to have the mix of both.

A pro of having multiple actions run is it may be a little more immediately clear what is failing when one of them does. But it also seems a little wasteful needing to perform the same environment setup for each action when they could be set up once and have all of them run.

The big pro, in my opinion, of having one make lint target for all types of linting is it makes it easier for the developer to run the checks locally before pushing up a change. Because everyone tests locally before doing that, right? ;)

@aman556
Copy link
Contributor Author

aman556 commented Jan 5, 2022

Thanks @stmcginnis and @vuil for following up agree what @stmcginnis mentioned if I am right then you are mentioning to run yaml linter from lint target only from where we are running licence linter i will update that but the one thing is making me confuse that for misspell we are running separate target as it is also a shell script so just want to confirm that we will move that too or I am missing something.

@navidshaikh
Copy link
Contributor

it also seems a little wasteful needing to perform the same environment setup for each action when they could be set up once and have all of them run.

The big pro, in my opinion, of having one make lint target for all types of linting is it makes it easier for the developer to run the checks locally before pushing up a change.

+1, all the lint checks can be triggered by a single Makefile target in main workflow. Also, instead of make lint we can use make check and remove an additional workflow for misspell.

@aman556 aman556 requested review from a team as code owners January 5, 2022 14:56
@github-actions
Copy link

github-actions bot commented Jan 5, 2022

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/1410/20220105150841/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@github-actions
Copy link

github-actions bot commented Jan 5, 2022

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/1410/20220105152507/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

- A bash script is added which is downloading yamllint docker image to check yamlfiles
- Config file for yaml linter is added in hack/linter dir

Signed-off-by: Aman Sharma <amansh@vmware.com>
- github workflow is removed for yaml lint

Signed-off-by: Aman Sharma <amansh@vmware.com>
Makefile Outdated Show resolved Hide resolved
hack/check/.yamllintconfig.yaml Outdated Show resolved Hide resolved
- trailing-spaces check is enabled and files are corrected
- new-line-at-end-of-file check is enabled and files are corrected

Signed-off-by: Aman Sharma <amansh@vmware.com>
@github-actions
Copy link

github-actions bot commented Jan 6, 2022

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/1410/20220106111429/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

Copy link
Contributor

@stmcginnis stmcginnis left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the clean up!

@navidshaikh
Copy link
Contributor

@vuil : This is enabling yaml linter and fixing the whitespaces and trailing new lines at the end of YAML files, would love to get this reviewed merged to avoid rebase cycles.

@stmcginnis stmcginnis added the ok-to-merge PRs should be labelled with this before merging label Jan 7, 2022
@stmcginnis stmcginnis merged commit e8a0831 into vmware-tanzu:main Jan 7, 2022
@Akasurde Akasurde mentioned this pull request Jan 10, 2022
10 tasks
@stmcginnis stmcginnis mentioned this pull request Jan 10, 2022
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-not-required ok-to-merge PRs should be labelled with this before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check: Integrate yaml linter
5 participants