[Code standards] Add detail on small PRs and Go packages #931

lbernick · 2023-01-13T16:17:57Z

This commit updates code standards to include guidance on pull request size, structuring Go packages, and better release notes.

It also removes language around being in a "releasable state".
It's difficult to write a PR that puts us in an unreleasable state, since our CI prevents this,
so guidance that PRs shouldn't put us in an unreleasable state adds confusion.

standards.md

bobcatfish

Looks great to me! 🎉 Very big fan of making standards like this clear, thanks @lbernick !

/approve

standards.md

lbernick · 2023-01-13T19:12:06Z

thanks @bobcatfish, just FYI I made a few changes clarifying our guidance around "releasable state" and partial feature development since you approved

vdemeester

😻

XinruZhang

Thanks @lbernick! Looks great! Just a minor comment.

standards.md

bendory · 2023-01-15T00:38:32Z

standards.md

@@ -160,33 +194,15 @@ what we want.
 * If possible, in addition to code snippets, include a reference to an end to end example
 * Ensure that all links and references are valid

-## Functionality


I would suggest keeping this section -- we do want to always be able to cut a release and should avoid merging a PR that puts us in an unreleasable state.

For incremental addition of new features, the incremental changes need to be made in a way that is non-breaking and non-blocking -- such as flag guards and the like.

I added a note that new features and api fields should be gated behind feature flags.
I agree we always want to be in an unreleasable state, but having this guidance in the code standards has caused some confusion since it's not clear how we'd get into an "unreleasable" state or what that actually means (thanks @XinruZhang for bringing this up!). I think it may be better to have specific guidance on what types of PRs to avoid. WDYT?

I'm not sure about removing this.

I think PR authors should ask themselves the question "if this was the last PR before the release, would we produce a usable release?". This is important to make sure that our nightly builds are usable as well as it helps very much in reducing the complexity the release manager may have to deal with.

Of course, CI takes a big role in ensuring that the code can be compiled and is generally safe and functionally sound. Feature flags help with incomplete features, but also documentation should be written so that it does not mislead users. We can try to make a complete list of all the things to consider, but I would prefer we still ask authors to ask themselves the question about the "releasable" state.

A few affirmative guidelines and / or antipatterns that I can think of:

No PR should require concurrent release of code in another repository

No PR should require that a release be held pending some later PR

No PR should include a breaking change without either (1) a controlling feature flag or (2) a release note that explains the intentionality of the breaking change

A PR that intentionally changes functionality should only be merged along with compatible, passing tests in the same PR

Thanks for the discussion here! I brought back the note about "unreleasable state" with a bit more explanation. @bendory I included all your examples but changed the third to "PRs must adhere to the project's API stability policy." I think it would be good as a follow up to move most of the contents of https://github.com/tektoncd/pipeline/blob/main/api_compatibility_policy.md that aren't specific to pipelines into the community repo so that all projects can link them.

/LGTM

modulo one minor nit in a separate comment

standards.md

tekton-robot · 2023-01-22T17:22:28Z

@bendory: changing LGTM is restricted to collaborators

In response to this:

Agree that it's a bit overspecified -- but I think it's helpful to overspecify for people new to this "small PR" principle in development.

/LGTM

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.

lbernick · 2023-01-27T17:50:36Z

@bobcatfish @vdemeester would either of you mind giving a LGTM please?

afrittoli

Thank a lot @lbernick - this looks good - only I disagree about removing the part about "releasable" state completely.

afrittoli · 2023-01-30T14:32:27Z

standards.md

@@ -160,33 +194,15 @@ what we want.
 * If possible, in addition to code snippets, include a reference to an end to end example
 * Ensure that all links and references are valid

-## Functionality


I'm not sure about removing this.

I think PR authors should ask themselves the question "if this was the last PR before the release, would we produce a usable release?". This is important to make sure that our nightly builds are usable as well as it helps very much in reducing the complexity the release manager may have to deal with.

Of course, CI takes a big role in ensuring that the code can be compiled and is generally safe and functionally sound. Feature flags help with incomplete features, but also documentation should be written so that it does not mislead users. We can try to make a complete list of all the things to consider, but I would prefer we still ask authors to ask themselves the question about the "releasable" state.

afrittoli

Thanks for the updates @lbernick - it looks good to me now!
/approve

standards.md

bendory · 2023-01-30T15:35:11Z

standards.md

@@ -160,33 +194,15 @@ what we want.
 * If possible, in addition to code snippets, include a reference to an end to end example
 * Ensure that all links and references are valid

-## Functionality


/LGTM

modulo one minor nit in a separate comment

tekton-robot · 2023-01-30T15:35:18Z

@bendory: changing LGTM is restricted to collaborators

In response to this:

/LGTM

modulo one minor nit in a separate comment

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.

This commit updates code standards to include guidance on pull request size, structuring Go packages, and better release notes. It also improves language around being in a "releasable state" and what kinds of PRs put us in an unreleasable state.

jerop

/lgtm

tekton-robot · 2023-02-01T16:15:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, bendory, bobcatfish, jerop, vdemeester, XinruZhang

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~~ [afrittoli,bobcatfish,vdemeester]

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

tekton-robot requested review from afrittoli and dibyom January 13, 2023 16:17

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 13, 2023

XinruZhang reviewed Jan 13, 2023

View reviewed changes

standards.md Outdated Show resolved Hide resolved

Yongxuanzhang reviewed Jan 13, 2023

View reviewed changes

standards.md Outdated Show resolved Hide resolved

standards.md Outdated Show resolved Hide resolved

lbernick force-pushed the standards branch from 6281b92 to 4dcd5ad Compare January 13, 2023 16:47

bobcatfish reviewed Jan 13, 2023

View reviewed changes

standards.md Outdated Show resolved Hide resolved

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 13, 2023

lbernick force-pushed the standards branch 3 times, most recently from 77c72be to e8c31ef Compare January 13, 2023 19:11

vdemeester approved these changes Jan 13, 2023

View reviewed changes

XinruZhang approved these changes Jan 13, 2023

View reviewed changes

standards.md Outdated Show resolved Hide resolved

lbernick force-pushed the standards branch from e8c31ef to 35ff9c0 Compare January 13, 2023 19:32

bendory approved these changes Jan 15, 2023

View reviewed changes

lbernick force-pushed the standards branch 2 times, most recently from aab9bf2 to 3aaf3ba Compare January 17, 2023 17:11

XinruZhang reviewed Jan 17, 2023

View reviewed changes

standards.md Show resolved Hide resolved

lbernick force-pushed the standards branch from 3aaf3ba to db5d30f Compare January 17, 2023 18:15

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 17, 2023

lbernick force-pushed the standards branch from db5d30f to 9d9571b Compare January 17, 2023 18:16

lbernick mentioned this pull request Jan 26, 2023

TEP-0100: PipelineRun Status Migration tektoncd/pipeline#4954

Closed

3 tasks

afrittoli reviewed Jan 30, 2023

View reviewed changes

lbernick force-pushed the standards branch from 9d9571b to 795f025 Compare January 30, 2023 15:16

afrittoli reviewed Jan 30, 2023

View reviewed changes

bendory approved these changes Jan 30, 2023

View reviewed changes

lbernick force-pushed the standards branch from 795f025 to 873f843 Compare January 30, 2023 16:20

lbernick force-pushed the standards branch from 873f843 to c3773a2 Compare February 1, 2023 13:55

jerop approved these changes Feb 1, 2023

View reviewed changes

tekton-robot assigned jerop Feb 1, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2023

tekton-robot merged commit facb27c into tektoncd:main Feb 1, 2023

lbernick mentioned this pull request Mar 1, 2023

Code standards: Add notes on releasable state #970

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Code standards] Add detail on small PRs and Go packages #931

[Code standards] Add detail on small PRs and Go packages #931

lbernick commented Jan 13, 2023 •

edited

Loading

bobcatfish left a comment

lbernick commented Jan 13, 2023

vdemeester left a comment

XinruZhang left a comment

bendory Jan 15, 2023

lbernick Jan 17, 2023

afrittoli Jan 30, 2023

bendory Jan 30, 2023

lbernick Jan 30, 2023

bendory Jan 30, 2023

tekton-robot commented Jan 22, 2023

lbernick commented Jan 27, 2023

afrittoli left a comment

afrittoli Jan 30, 2023

afrittoli left a comment

bendory Jan 30, 2023

tekton-robot commented Jan 30, 2023

jerop left a comment

tekton-robot commented Feb 1, 2023

[Code standards] Add detail on small PRs and Go packages #931

[Code standards] Add detail on small PRs and Go packages #931

Conversation

lbernick commented Jan 13, 2023 • edited Loading

bobcatfish left a comment

Choose a reason for hiding this comment

lbernick commented Jan 13, 2023

vdemeester left a comment

Choose a reason for hiding this comment

XinruZhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Jan 22, 2023

lbernick commented Jan 27, 2023

afrittoli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afrittoli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Jan 30, 2023

jerop left a comment

Choose a reason for hiding this comment

tekton-robot commented Feb 1, 2023

lbernick commented Jan 13, 2023 •

edited

Loading