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

Collect PR review rules for documentation and automation #2373

Closed
mattfarina opened this issue Oct 2, 2017 · 12 comments
Closed

Collect PR review rules for documentation and automation #2373

mattfarina opened this issue Oct 2, 2017 · 12 comments
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.

Comments

@mattfarina
Copy link
Contributor

When a chart PR is reviewed there are common items reviewers check for. I would like to collect those to document and try to automate them. Can we get a list?

/cc @unguiculus @viglesiasce
@unguiculus
Copy link
Member

You should start looking at best practices.

https://github.com/kubernetes/helm/tree/master/docs/chart_best_practices

One of the first things I look at is resource names and standard labels.

@viglesiasce
Copy link
Contributor

For me:

  1. Version bump for updates to existing charts
  2. Chart.yaml has a github ID for name (in transition)
  3. Images are all configurable
  4. NOTES.txt is present and accurate/useful

Also:
https://github.com/kubernetes/charts/blob/master/CONTRIBUTING.md#technical-requirements

@mattfarina mattfarina mentioned this issue Oct 4, 2017
Closed
@prydonius
Copy link
Member

There are a couple of things we can add to Helm lint, and other things that are more specific to k8s/charts, so we should figure out what fits where. Off the top of my head:

Linter:

  • naming conventions for chart names, resources, namespaced template names etc.
  • semver versions (I believe this is already implemented)
  • YAML formatting (e.g. 2 space indentation)
  • Images are all configurable
  • Chart.yaml fields (should fail on "strict"): home, sources, icons, appVersion

k8s/charts specific:

  • ensure version bump
  • GitHub ID for maintainer name
  • NOTES.txt present
  • check that requirements.yaml only references charts in the stable repo (or also incubator only if the chart itself is in incubator)

Not sure:

  • Labels - whilst the chart best practices define a set of standard labels, I'm not sure that the linter should necessarily enforce this. I think we have the choice here of whether it's a "strict" linter rule (Info/Warning) or whether we only implement this as a check for k8s/charts only.

Things that probably still need to be manual:

  • Version bump appropriate
  • NOTES.txt accurate/useful

@mattfarina
Copy link
Contributor Author

@unguiculus @viglesiasce @prydonius For charts in the stable directory, should their chart version be > 0. Note, the semver spec makes note of versions starting with 0.x.y at http://semver.org/#spec-item-4.

@unguiculus
Copy link
Member

Maybe we should work on a clear definition for stable and incubator first.

@unguiculus
Copy link
Member

Now that helm/helm#2845 has been merged, I'd suggest to check for namespaced templates for new charts once Helm 2.7 is out. However, helm lint is not configurable. What about making it configurable with linting rules as other linters do? This would allow us to apply different rules for new and for existing charts.

@mattfarina mattfarina mentioned this issue Oct 6, 2017
Merged
@mattfarina
Copy link
Contributor Author

@unguiculus isn't everything in the stable directory supposed to be stable? And stable in SemVer gets a >=1.0.0 version. Are there things in stable that aren't considered stable.

I understand a lack of formal definition. But, from the perspective of a consumer... stable is stable.

@mattfarina mattfarina mentioned this issue Oct 10, 2017
Merged
@mattfarina mattfarina mentioned this issue Oct 11, 2017
Merged
mattfarina added a commit to mattfarina/charts that referenced this issue Oct 11, 2017
@mattfarina
CI Check that a chart version was incremented
f4ee80a 
This commit does a couple things
1. It reformats the layout to have checks via self contained
   functions rather than all in one loop.
2  Adds a check that the chart.version was incremented

Ref helm#2373
prydonius pushed a commit that referenced this issue Oct 12, 2017
@mattfarina @prydonius
CI Check that a chart version was incremented (#2462)
f085700 
This commit does a couple things
1. It reformats the layout to have checks via self contained
   functions rather than all in one loop.
2  Adds a check that the chart.version was incremented

Ref #2373
mattfarina added a commit to mattfarina/charts that referenced this issue Oct 12, 2017
@mattfarina
Adding a CI check for the NOTES.txt file
cf6c682 
Points to docs when file missing to point user to a fix

Ref helm#2373
prydonius pushed a commit that referenced this issue Oct 13, 2017
@mattfarina @prydonius
Adding a CI check for the NOTES.txt file (#2453)
8ca39ff 
Points to docs when file missing to point user to a fix

Ref #2373
@mattfarina
Copy link
Contributor Author

Folks, any thoughts on #2528? I could automate some of those.

dnelson pushed a commit to Vungle/charts that referenced this issue Oct 26, 2017
@mattfarina @dnelson
CI Check that a chart version was incremented (helm#2462)
6f4a704 
This commit does a couple things
1. It reformats the layout to have checks via self contained
   functions rather than all in one loop.
2  Adds a check that the chart.version was incremented

Ref helm#2373
dnelson pushed a commit to Vungle/charts that referenced this issue Oct 26, 2017
@mattfarina @dnelson
Adding a CI check for the NOTES.txt file (helm#2453)
7530689 
Points to docs when file missing to point user to a fix

Ref helm#2373
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

Prevent issues from auto-closing with an /lifecycle frozen comment.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 18, 2018
@mattfarina mattfarina removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 6, 2018
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 7, 2018
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jun 6, 2018
@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 6, 2018
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@mattfarina @viglesiasce @unguiculus @prydonius @k8s-ci-robot @fejta-bot