calico icon indicating copy to clipboard operation
calico copied to clipboard

Add yamllint configuration for generated manifests

Open jayaddison opened this issue 3 years ago • 3 comments

Description

Please note: this is very low priority modification!

  • A yamllint configuration is added to the base directory, intended for use with the manifests directory.
  • Two rule fixups are applied to static files in the repository (mostly in the manifests directory, although some of the changes were equally applied to the same template content found in the charts and node directories).
  • No modifications are yet made to the Makefiles, but this should perhaps be added at the same time (I wasn't sure where to add this)

Rationale: the original motivation was noticing during git diff review between versions that some of the YAML files contain trailing spaces - purely a review-time distraction.

Todos

  • [ ] Integrate with one or more makefiles

Release Note

None

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

  • docs-pr-required: This change requires a change to the documentation that has not been completed yet.
  • docs-completed: This change has all necessary documentation completed.
  • docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

  • release-note-required: This PR has user-facing changes. Most PRs should have this label.
  • release-note-not-required: This PR has no user-facing changes.

Other optional labels:

  • cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
  • needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

jayaddison avatar Jun 07 '22 15:06 jayaddison

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Jun 07 '22 15:06 CLAassistant

:wave: Hey @jayaddison !

I think a yaml linter would be nice to have (I hate extra spaces in my yaml).

One thing about this is that it seems to be running the yaml linting after the manifest generation - is that right? I don't see any changes to the charts/calico or charts/tigera-operator directories, which is the source material for these manifests, which means that any future updates will result in us undoing the cleanups (unless we also automatically run the linter as part of the manifest generation, but I don't see logic in this PR to do that).

Do you know if the linter will work on template files that include yaml?

caseydavenport avatar Jun 14 '22 16:06 caseydavenport

Thanks @caseydavenport - I probably should've started this off by filing a suggestion/feature request, and working from there.

yamllint does barf on the (Helm?) source templates due to some of the non-yaml syntax unfortunately, so yep, I was running it on the generated output, and indeed perhaps it'd make sense to run the linting as a post-manifest-generation step. I have had a little more of a look at the generate.sh script since opening the pull request, so it's starting to make a bit more sense.

Another thing I'm bearing in mind is that the codebase seems fairly golang-oriented, and so perhaps it'd be sensible to use a lint tool within the same ecosystem (I see that kube-linter is a thing, although I don't have experience with it yet).

jayaddison avatar Jun 14 '22 22:06 jayaddison

Closing as stale (with a bit of luck I'll try a more thorough approach in future, but that's unplanned at the moment).

jayaddison avatar Nov 02 '22 01:11 jayaddison

@jayaddison yeah, sorry for letting this slip! I think it would be nice to have something like this, but it definitely needs to be integrated with the manifest generation logic in generate.sh so that we don't lose the cleanups next time we make changes.

kube-linter looks compelling at a glance, although I also haven't used it before.

caseydavenport avatar Nov 02 '22 17:11 caseydavenport