Enhancing lint.sh with additional checks for Chart.yaml #4161

depohmel · 2018-03-15T00:14:26Z

This PR is enhancing current CirckeCi initial linter.

It looks like most of the time submitted Chart.yaml (a super common case for new charts) has missing fields. this PR is enabling field validation against yaml-schemas/Chart.yaml schema file with https://github.com/23andMe/Yamale CLI tool.
Also, this change will allow adding schema[s] for values.yaml.

Maintainer GitHub account validation is another new test. List of maintainers got parsed with yq tool and validated via curl against https://github.com/<account_name>
this should make @unguiculus life slightly easy.

Examples of outputs:
Successful validation

circleci@515ce1f45960:/charts$ sudo test/circle/lint.sh stable/kanister-operator/
From https://github.com/kubernetes/charts
 * branch            master     -> FETCH_HEAD
test/circle/lint.sh: line 78: Exit: command not found

Running helm dep build on the chart at stable/kanister-operator
No requirements found in stable/kanister-operator/charts.

Running helm lint on the chart at stable/kanister-operator
==> Linting stable/kanister-operator
Lint OK

1 chart(s) linted, no failures

Linting the Chart.yaml and values.yaml files at stable/kanister-operator
Validating /charts/stable/kanister-operator/Chart.yaml...
Validation success! 👍
Validating maintainers names

Checking the Chart version has increased for the chart at stable/kanister-operator
New higher version 0.2.1 found

Missing field in Chart.yaml

==> Linting stable/weave-cloud
Lint OK

1 chart(s) linted, no failures

Linting the Chart.yaml and values.yaml files at stable/weave-cloud
Validating /charts/stable/weave-cloud/Chart.yaml...

Error!
Schema: test/circle/yaml-schemas/Chart.yaml
Data file: /charts/stable/weave-cloud/Chart.yaml
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/site-packages/yamale/command_line.py", line 29, in _validate
    yamale.validate(schema, data)
  File "/usr/local/lib/python3.6/site-packages/yamale/yamale.py", line 39, in validate
    schema.validate(d)
  File "/usr/local/lib/python3.6/site-packages/yamale/schema/schema.py", line 56, in validate
    raise ValueError(error_str)
ValueError: 
Error validating data /charts/stable/weave-cloud/Chart.yaml with schema test/circle/yaml-schemas/Chart.yaml
	appVersion: Required field missing

incorrect maintainer name

==> Linting stable/kanister-operator
Lint OK

1 chart(s) linted, no failures

Linting the Chart.yaml and values.yaml files at stable/kanister-operator
Validating /charts/stable/kanister-operator/Chart.yaml...
Validation success! 👍
Validating maintainers names
Error: depohmel65 is not valid GitHub account

depohmel · 2018-03-15T00:16:07Z

/assign @unguiculus
/assign @mattfarina
/assign @viglesiasce

viglesiasce · 2018-03-18T17:54:05Z

test/circle/lint.sh

 # include the semvercompare function
 curDir="$(dirname "$0")"
 source "$curDir/../semvercompare.sh"
-
-git remote add k8s https://github.com/kubernetes/charts
+#git remote add k8s https://github.com/kubernetes/charts


Was this removed intentionally?

I modified the file for local testing. this is post revert leftovers

viglesiasce · 2018-03-18T17:54:18Z

test/circle/lint.sh

 git fetch k8s master
 CHANGED_FOLDERS=`git diff --find-renames --name-only $(git merge-base k8s/master HEAD) stable/ incubator/ | awk -F/ '{print $1"/"$2}' | uniq`

-# Exit early if no charts have changed
+Exit early if no charts have changed


This was uncommented for some reason

mattfarina

In general, I like the additional checks and where this is going. Thanks for putting this together. I just have a little feedback.

mattfarina · 2018-03-19T13:34:21Z

test/circle/install.sh

+
+# Install a yq bash yaml cli tool
+# Pinning to a version for consistency
+sudo pip install yq==2.1.1


The tool installed below (https://github.com/mikefarah/yaml) is also a yaml query tool. It's been renamed yq (we're still using an older version). We should install one yaml querying tool and use that consistently.

I'm indifferent to the one we use.

mattfarina · 2018-03-19T13:36:14Z

test/circle/lint.sh

+
+  for name in $(yq -r '.maintainers|.[]|.name' ${1}/Chart.yaml); do
+    if [ $(curl -s -o /dev/null -w "%{http_code}\n" -If https://github.com/${name}) -ne 200 ]; then
+      echo "Error: ${name} is not valid GitHub account"


I like this check. Would it be possible to add some polite language letting people know we have that as a requirement on the charts repo so that we can contact chart maintainers in issues/PRs.

mattfarina · 2018-03-19T13:36:49Z

test/circle/lint.sh

 git remote add k8s https://github.com/kubernetes/charts
 git fetch k8s master
 CHANGED_FOLDERS=`git diff --find-renames --name-only $(git merge-base k8s/master HEAD) stable/ incubator/ | awk -F/ '{print $1"/"$2}' | uniq`

-# Exit early if no charts have changed
+#Exit early if no charts have changed


The space before Exit was removed?

mattfarina · 2018-03-19T13:45:38Z

test/circle/yaml-schemas/Chart.yaml

+---
+maintainer:
+  name: str()
+  email: str(required=False)


There are more than just these that can be in a chart. The options are described in a proto file. Could we get more of the properties added to the schema?

removing yq installetion chaning yq with yaml for Chart.yaml parsing. modifing Erro message for not github account check. also modifing lint.sh so it can be used locally

depohmel · 2018-03-20T20:28:53Z

I've modified lint.sh so it can be used locally. it can be removed and added as separate, better-structured PR.
Once $1 arg is specified, all checks will be run against that folder.

I was trying to add something like lint_my_chart.sh, but got stuck with questions: where to place it, where should readme go, what execution to support etc
for now, it can be used with following CMD:
from charts folder

docker run -ti -v $(pwd .):/charts -w /charts/test circleci/python:3.6.4 bash -c "sudo circle/install.sh && circle/lint.sh ../stable/weave-cloud"

viglesiasce · 2018-03-20T21:49:49Z

/approve

viglesiasce · 2018-03-20T21:50:02Z

/ok-to-test

mattfarina · 2018-03-27T16:27:03Z

/lgtm

k8s-ci-robot · 2018-03-27T16:27:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: depohmel, mattfarina, viglesiasce

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mattfarina,viglesiasce]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* Enhancing lint.sh with additional checks * reverting post local testing changes * addressing reviews. removing yq installetion chaning yq with yaml for Chart.yaml parsing. modifing Erro message for not github account check. also modifing lint.sh so it can be used locally

* Enhancing lint.sh with additional checks * reverting post local testing changes * addressing reviews. removing yq installetion chaning yq with yaml for Chart.yaml parsing. modifing Erro message for not github account check. also modifing lint.sh so it can be used locally Signed-off-by: voron <av@arilot.com>

Enhancing lint.sh with additional checks

6bf041e

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 15, 2018

k8s-ci-robot assigned mattfarina, unguiculus and viglesiasce Mar 15, 2018

viglesiasce suggested changes Mar 18, 2018

View reviewed changes

reverting post local testing changes

6b779bf

mattfarina reviewed Mar 19, 2018

View reviewed changes

addressing reviews.

8767e77

removing yq installetion chaning yq with yaml for Chart.yaml parsing. modifing Erro message for not github account check. also modifing lint.sh so it can be used locally

viglesiasce approved these changes Mar 20, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 20, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 20, 2018

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2018

k8s-ci-robot merged commit bd8016c into helm:master Mar 27, 2018

depohmel deleted the enhance-linter branch March 27, 2018 16:33

This was referenced Apr 3, 2018

[chartmuseum] move from incubator to stable #4601

Merged

[linting] Temporarily make maintainers optional for linting #4663

Merged

Add home key for magic-namespace #4696

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancing lint.sh with additional checks for Chart.yaml #4161

Enhancing lint.sh with additional checks for Chart.yaml #4161

depohmel commented Mar 15, 2018

depohmel commented Mar 15, 2018

viglesiasce Mar 18, 2018

depohmel Mar 19, 2018

viglesiasce Mar 18, 2018

mattfarina left a comment

mattfarina Mar 19, 2018

mattfarina Mar 19, 2018

mattfarina Mar 19, 2018

mattfarina Mar 19, 2018

depohmel commented Mar 20, 2018 •

edited

Loading

viglesiasce commented Mar 20, 2018

viglesiasce commented Mar 20, 2018

mattfarina commented Mar 27, 2018

k8s-ci-robot commented Mar 27, 2018

Enhancing lint.sh with additional checks for Chart.yaml #4161

Enhancing lint.sh with additional checks for Chart.yaml #4161

Conversation

depohmel commented Mar 15, 2018

depohmel commented Mar 15, 2018

viglesiasce Mar 18, 2018

Choose a reason for hiding this comment

depohmel Mar 19, 2018

Choose a reason for hiding this comment

viglesiasce Mar 18, 2018

Choose a reason for hiding this comment

mattfarina left a comment

Choose a reason for hiding this comment

mattfarina Mar 19, 2018

Choose a reason for hiding this comment

mattfarina Mar 19, 2018

Choose a reason for hiding this comment

mattfarina Mar 19, 2018

Choose a reason for hiding this comment

mattfarina Mar 19, 2018

Choose a reason for hiding this comment

depohmel commented Mar 20, 2018 • edited Loading

viglesiasce commented Mar 20, 2018

viglesiasce commented Mar 20, 2018

mattfarina commented Mar 27, 2018

k8s-ci-robot commented Mar 27, 2018

depohmel commented Mar 20, 2018 •

edited

Loading