Proposal: Lint CI on every PR automatically

[mattfarina]

Currently, prior to any testing of a pull request a kubernetes org member needs to add a comment of /ok-to-test to the PR. At the time I'm writing this issue there are 190 open PRs waiting for this to happen. The testing consists of linting, smell testing (installs the chart), and helm testing (running tests if they are available). For security purposes, the smell and helm testing should not happen automatically but should be run after someone has checked out the PR (e.g., to make sure someone didn't create a PR for a bitcoin miner).

But, linting of a chart is safe to fun on every PR. If there are failures those can be reported back immediately and we don't need to wait for a maintainer to act.

The proposal is to add automated testing on every PR that performs a helm lint on changed charts.

This can be done rather simply using CircleCI or Travis CI using their free tier. The configuration stored in the charts repo and a webhook used to fire off the notification to test.

Note, I don't suggest this for someone else to do. If acceptable and with the proper access I would be willing to set this up.

/cc @prydonius @viglesiasce

[prydonius]

I agree that we should lint without ok-to-test being required, but I believe the biggest reason for ok-to-test is to make sure no one is able to submit a malicious PR that changes the test scripts. AFAIK the k8s CI will run the scripts from the PR branch and not the master branch and I don't know that there is a way to configure this. Does Travis or Circle circumvent this by only using the master branch's test configuration?

[mattfarina]

@prydonius They don't run the master branches test configuration. They run that which is on the PR. This in intentional for the cases where you want the test config to go along with the change so that they are in sync.

But, open source projects using a system like this to perform tests is fairly common. Even Kubernetes projects do it today. For example, glide, dep, helm, kubeless, monocular, kops, kompose, Kubernetes Dashboard, and minikube are just a few projects using either Travis CI (in most cases) or CircleCI to handle some of their testing.

Is there a reason that charts should be wary of a system like this when other Kubernetes projects are not?

[viglesiasce]

The reason we are wary is because we run functional validation in our checks. We should separate the helm lint from the functional validation. The fact that we do run from their testing configuration is exactly the reason why we should be cautious before doing heavy testing. Someone could submit something that removes the test steps altogether and that would look like a passing PR.

We should open another issue to decide what can/should go into helm lint. ATM it is very very light and not very opinionated. We should find a way to add our opinions to it so that maintainers have less manual code review to do. Its hard to keep all the best practices in your head when reviewing, especially since they change regularly. It would also be useful for our contributors to be able to run more strict tests locally so that they aren't surprised when we review.

Helm lint should be the contract that we can automate between maintainers and contributors.

[mattfarina]

@viglesiasce @prydonius Can you take a look at https://github.com/mattfarina/charts/commit/982f3822c6b430773c9ce8a4e1f56700e346a9d8 and you can see the circleci output at https://circleci.com/gh/mattfarina/charts/27

This is not PR ready but rather test code. I would be interested in feedback on the style setup.

[technosophos]

I'm not sure I am totally following this... but it does seem like having lint (even as it is today) run immediately on all PRs would be a good thing. Could we use a Circle or Travis pull_request hook to do just the linting, and have that be automatic? (And still have a comment-based trigger for the full testing run)

[viglesiasce]

@technosophos thats the plan.

[mattfarina]

Now that additional elements are being collected on #2373 and a framework is in place to do them I'm going to close this one.