release icon indicating copy to clipboard operation
release copied to clipboard
verified publisher icon kubernetes

Add style guide and code quality standards

Open hasheddan opened this issue 5 years ago • 17 comments

What would you like to be added:

As the functionality and number of packages in krel continues to grow, we are starting to see divergence in how new features are implemented, tested, and maintained. We should add style guidelines and quality standards so that someone is completely new to the project can make contributions that are easily reviewed and integrated without degrading quality.

Why is this needed:

There are a few key areas that we have determined as important to maintain quality and consistency:

  • Exposing the minimum required public API in packages (i.e. bias towards private fields in structs and interfaces)
  • Follow pattern of "accepting interfaces, returning structs" (i.e Robustness principle)
  • Avoid "testing through" (i.e. make your interfaces easily mockable)
  • Generate mocks in a consistent manner (we use counterfeiter)
  • Establish and measure a (potentially flexible) threshold for unit test coverage
  • Establish what types of features require integration / e2e testing

This is not an all-inclusive list, but can serve as a good starting point for a document that we can continually iterate on.

/assign @hasheddan @saschagrunert

hasheddan avatar Nov 06 '20 16:11 hasheddan

Thanks for opening this, Dan! /priority important-soon /milestone v1.21

justaugustus avatar Nov 06 '20 16:11 justaugustus

+1 Thanks for pointing this out dan, I certainly would like to get some some of these well established (and understood!) to start implementing them. Sometimes I get lost and I feel waste too much time reinventing the wheel.

puerco avatar Nov 06 '20 17:11 puerco

@hasheddan when do we want to work on this issue? Seems not part of our roadmap planning but we may wire it into some side-effort.

saschagrunert avatar Dec 11 '20 09:12 saschagrunert

Fun discussion w/ @wilsonehusin and some of the team on Twitter: https://twitter.com/stephenaugustus/status/1359354950019657730?s=20

Let's try and tackle this as a priority for v1.21.

justaugustus avatar Feb 10 '21 04:02 justaugustus

/assign @puerco @wilsonehusin /unassign @hasheddan @saschagrunert /milestone v1.22

justaugustus avatar Mar 24 '21 18:03 justaugustus

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-contributor-experience at kubernetes/community. /lifecycle stale

fejta-bot avatar Jun 22 '21 19:06 fejta-bot

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-contributor-experience at kubernetes/community. /lifecycle rotten

fejta-bot avatar Jul 22 '21 19:07 fejta-bot

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-triage-robot avatar Aug 21 '21 20:08 k8s-triage-robot

@k8s-triage-robot: Closing this issue.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot avatar Aug 21 '21 20:08 k8s-ci-robot

I was thinking about this again, thanks to @helayoty! (ref: https://twitter.com/helayoty/status/1497366986841460738) /reopen

@kubernetes/release-engineering -- Thoughts here?

justaugustus avatar Feb 26 '22 00:02 justaugustus

@justaugustus: Reopened this issue.

In response to this:

I was thinking about this again, thanks to @helayoty! (ref: https://twitter.com/helayoty/status/1497366986841460738) /reopen

@kubernetes/release-engineering -- Thoughts here?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot avatar Feb 26 '22 00:02 k8s-ci-robot

These are probably a good start:

  • https://www.kubernetes.dev/docs/guide/coding-convention/
  • https://www.pullrequest.com/blog/create-a-programming-style-guide/
  • https://google.github.io/styleguide/
  • https://github.com/golang/go/wiki/CodeReviewComments
  • https://go.dev/doc/effective_go

justaugustus avatar Feb 26 '22 01:02 justaugustus

/unassign @wilsonehusin @puerco /assign @justaugustus @helayoty

Heba, I'm going to take an initial pass using the awesome Crossplane one @hasheddan linked: https://github.com/crossplane/crossplane/blob/master/CONTRIBUTING.md

From there, we can iterate on improvements together!

justaugustus avatar Feb 26 '22 02:02 justaugustus

/unassign @wilsonehusin @puerco /assign @justaugustus @helayoty

Heba, I'm going to take an initial using the awesome Crossplane one @hasheddan linked: https://github.com/crossplane/crossplane/blob/master/CONTRIBUTING.md

From there, we can iterate on improvements together!

While it's so hard now to top this amazing work, I totally agree.

helayoty avatar Feb 26 '22 02:02 helayoty

Heba, I'm going to take an initial pass using the awesome Crossplane one @hasheddan linked: https://github.com/crossplane/crossplane/blob/master/CONTRIBUTING.md

First pass: https://github.com/kubernetes/sig-release/pull/1862

justaugustus avatar Feb 26 '22 06:02 justaugustus

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot avatar May 27 '22 06:05 k8s-triage-robot

@justaugustus the first pass form your last comment is merged.

Would a follow up for this be to reference the k/sig-release contributing doc on all sig release repos in the CONTRIBUTING.md so its clear there is sort of one central contributing guide for sig release.

Would that be basically every project in https://github.com/kubernetes/community/tree/master/sig-release#subprojects?

matglas avatar Jun 08 '22 14:06 matglas