From fd70ad2cf1d255eb24f4ca03e913ebf3a903cbbf Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Wed, 2 Oct 2019 17:26:47 -0700 Subject: [PATCH 1/5] Add small pr guidelines --- DEVELOPMENT.md | 1 + docs/community/small-prs.md | 75 +++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+) create mode 100644 docs/community/small-prs.md diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index 11b32015fba..c04113c304d 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -243,6 +243,7 @@ When you have changes you would like to propose to skaffold, you will need to: 1. Add unit tests. Unit test coverage should increase or stay the same with every PR. 1. [Create a pull request](https://help.github.com/articles/creating-a-pull-request-from-a-fork/) +Please follow our [small Pull Requests guidelines](./docs/community/small-prs.md) for quicker response time. ### Reviews Each PR must be reviewed by a maintainer. This maintainer will add the `kokoro:run` label diff --git a/docs/community/small-prs.md b/docs/community/small-prs.md new file mode 100644 index 00000000000..d0d494a570d --- /dev/null +++ b/docs/community/small-prs.md @@ -0,0 +1,75 @@ +# Pull Request Guidelines + +Writing good pull requests increase development velocity and minimize frustration. + +If pull requests are small they will result in being opened and merged quickly. + +Having big pull requests that bounce back and forth between developers +and reviewers can slow progress down significantly, causing developers to waste a +lot of time dealing with merge conflicts. + +Big pull requests also increase the risk of breaking things; since changes are so big, + - it’s hard to scrutinize all the changes and + - test them for regressions. + +In order to promote good PR quality, the skaffold team will push back +against big PRs. See [0](https://github.com/GoogleContainerTools/skaffold/pull/2750#pullrequestreview-283621241), [1](https://github.com/GoogleContainerTools/skaffold/pull/2917#pullrequestreview-291415741) + +## Breaking down Pull Requests + +### Feature development. +Adding a new command or skaffold config results in large number of changes due to +all the boilerplate code required for adding a new command, generating docs etc. + +It makes sense to break down these changes into 2 categories: +1. Incremental code changes which are not accessed by control flow. +2. User affecting changes like + 1. Adding a new command which exercises the above code changes + 1. Adding config change and generating docs. + +#### Config Change Example +Below is an example of how you can break a config change into small individual PRs. + +A contributor wanted to support [custom build args to `kustomize` deployer](https://github.com/GoogleContainerTools/skaffold/issues/2488). +This required, skaffold to support a `buildArgs` field to `deploy.kustomize` config. Plumb the config to `kustomize` command. + +This small 10 line change results into 100+ lines of code along with test code. +It would greatly simplify code review process if we break this change into +1. Coding logic and test coverage. +2. User facing documentation. + +In this case, the contributor split this PR in 2 small changes +1. Introduce a [place holder for a new config](https://github.com/GoogleContainerTools/skaffold/pull/2870). + + This PR highlights the logic changes and makes it easier for reviewers to review control logic and tests. + + Note: The code added in this PR does not get exercised other than in tests. +2. [Add a field to skaffold config](https://github.com/GoogleContainerTools/skaffold/pull/2871) and pass it to the place holder. + This PR makes it easier for reviewers to review the user facing config changes and make sure all the precautions like + - good user documentation + - deprecation policy if applicable + - upgrade policy etc were followed. + +#### Adding new functionality +Below is an example on how you can introduce a new functionality to skaffold in smaller incremental PRs. + +To add ability to [render templated k8 manifests without deploying](https://github.com/GoogleContainerTools/skaffold/issues/1187), required us to make a bunch of changes in skaffold +1. Add this feature to `kubectl`, `helm` and `kustomize` deployers supported by skaffold. +2. support a new flag for `skaffold deploy` and `skaffold run` command +3. may be add a new command to only render k8 manifests. + +Item 2 and 3 in the above list of changes, are user facing features and could be abstracted out. +Item 1 can further be implemented incrementally by supporting this new feature for each of the deployer. + +To implement this feature, we +1. First added an [method to interface `deployer` with empty stub](https://github.com/GoogleContainerTools/skaffold/pull/2834). + + Note: The code added in this PR does not get exercised other than in tests. +2. Then add render implementation for each [kubectl deployer](https://github.com/GoogleContainerTools/skaffold/pull/2943), kustomize and helm. +3. Add a [render command](https://github.com/GoogleContainerTools/skaffold/pull/2942) + +Remember, its ok to land code which is not exercised. However please mention the follow up work +in **Next PRs** section when creating a Pull Request [e.g. here.](https://github.com/GoogleContainerTools/skaffold/pull/2811) + + +Finally, please use our best judgement when submitting pull requests. From 0a110ef0f07f5f77db78b70cccb30999d191e3d9 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Wed, 2 Oct 2019 18:57:05 -0700 Subject: [PATCH 2/5] small fix. --- docs/community/small-prs.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/community/small-prs.md b/docs/community/small-prs.md index d0d494a570d..8ed2e069224 100644 --- a/docs/community/small-prs.md +++ b/docs/community/small-prs.md @@ -41,7 +41,7 @@ It would greatly simplify code review process if we break this change into In this case, the contributor split this PR in 2 small changes 1. Introduce a [place holder for a new config](https://github.com/GoogleContainerTools/skaffold/pull/2870). - This PR highlights the logic changes and makes it easier for reviewers to review control logic and tests. + This PR highlights the logic changes and makes it easier for reviewers to review code logic and tests. Note: The code added in this PR does not get exercised other than in tests. 2. [Add a field to skaffold config](https://github.com/GoogleContainerTools/skaffold/pull/2871) and pass it to the place holder. From a8ddaa015fb38bb29959ed0ac6194cb0d9188e30 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Thu, 3 Oct 2019 22:55:19 -0700 Subject: [PATCH 3/5] Apply suggestions from @balopat code review changes. Co-Authored-By: Balint Pato --- docs/community/small-prs.md | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/docs/community/small-prs.md b/docs/community/small-prs.md index 8ed2e069224..5bb3fa05d47 100644 --- a/docs/community/small-prs.md +++ b/docs/community/small-prs.md @@ -2,14 +2,14 @@ Writing good pull requests increase development velocity and minimize frustration. -If pull requests are small they will result in being opened and merged quickly. +Small pull request will be merged quicker. Having big pull requests that bounce back and forth between developers and reviewers can slow progress down significantly, causing developers to waste a lot of time dealing with merge conflicts. Big pull requests also increase the risk of breaking things; since changes are so big, - - it’s hard to scrutinize all the changes and + - it’s hard to truly understand all the changes and - test them for regressions. In order to promote good PR quality, the skaffold team will push back @@ -17,13 +17,15 @@ against big PRs. See [0](https://github.com/GoogleContainerTools/skaffold/pull/2 ## Breaking down Pull Requests -### Feature development. +### Feature development Adding a new command or skaffold config results in large number of changes due to all the boilerplate code required for adding a new command, generating docs etc. It makes sense to break down these changes into 2 categories: -1. Incremental code changes which are not accessed by control flow. -2. User affecting changes like +1. "Invisible changes": Incremental, small code changes + 1. refactoring: either keep the functionality the same but refactoring the design or + 1. partial implementations: unit tested, new behavior, that is not yet accessible by the main control flow. +2. "Visible changes": User affecting changes, for example 1. Adding a new command which exercises the above code changes 1. Adding config change and generating docs. @@ -31,7 +33,7 @@ It makes sense to break down these changes into 2 categories: Below is an example of how you can break a config change into small individual PRs. A contributor wanted to support [custom build args to `kustomize` deployer](https://github.com/GoogleContainerTools/skaffold/issues/2488). -This required, skaffold to support a `buildArgs` field to `deploy.kustomize` config. Plumb the config to `kustomize` command. +This required skaffold to add the `deploy.kustomize.buildArgs` field and then plumb the arguments to the `kustomize` command. This small 10 line change results into 100+ lines of code along with test code. It would greatly simplify code review process if we break this change into @@ -72,4 +74,4 @@ Remember, its ok to land code which is not exercised. However please mention the in **Next PRs** section when creating a Pull Request [e.g. here.](https://github.com/GoogleContainerTools/skaffold/pull/2811) -Finally, please use our best judgement when submitting pull requests. +Finally, please use your best judgement when submitting pull requests, these rules might not always work for you - we would love to hear that! From e7577b737a9db5340a5f45cead13092ee6f9bf65 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Thu, 3 Oct 2019 23:20:47 -0700 Subject: [PATCH 4/5] david's changes. --- docs/community/small-prs.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/community/small-prs.md b/docs/community/small-prs.md index 5bb3fa05d47..e78d87edabc 100644 --- a/docs/community/small-prs.md +++ b/docs/community/small-prs.md @@ -1,6 +1,6 @@ # Pull Request Guidelines -Writing good pull requests increase development velocity and minimize frustration. +Writing good pull requests increases development velocity and minimizes frustration. Small pull request will be merged quicker. @@ -73,5 +73,4 @@ To implement this feature, we Remember, its ok to land code which is not exercised. However please mention the follow up work in **Next PRs** section when creating a Pull Request [e.g. here.](https://github.com/GoogleContainerTools/skaffold/pull/2811) - Finally, please use your best judgement when submitting pull requests, these rules might not always work for you - we would love to hear that! From 625fc1fc0fd1c2197a4bfb6bcbaaed2678961718 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Mon, 7 Oct 2019 11:21:20 -0700 Subject: [PATCH 5/5] add notes for draft pr --- docs/community/small-prs.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/docs/community/small-prs.md b/docs/community/small-prs.md index d0d494a570d..32ee952c222 100644 --- a/docs/community/small-prs.md +++ b/docs/community/small-prs.md @@ -18,6 +18,8 @@ against big PRs. See [0](https://github.com/GoogleContainerTools/skaffold/pull/2 ## Breaking down Pull Requests ### Feature development. +When proposing a new feature, please review our [Design Document Proposal](../design_proposals) to see if you should first create a `Design Proposal`. + Adding a new command or skaffold config results in large number of changes due to all the boilerplate code required for adding a new command, generating docs etc. @@ -72,4 +74,12 @@ Remember, its ok to land code which is not exercised. However please mention the in **Next PRs** section when creating a Pull Request [e.g. here.](https://github.com/GoogleContainerTools/skaffold/pull/2811) +Implementing the full functionality sometimes might makes a lot of sense, + - so that you can get feedback regarding the code while implementing it also + - the maintainers can try it out and test it, get a feel for it. + +If you are opening a big PR, we can mark these as `Draft PR` that will broken down into smaller PRs that can refer back to this `Draft PR`. +You can either rebase the `Draft PR` as the smaller pieces get merged, and then finally merge `Draft PR` or close it without merging once all functionality is implemented. +See for example [#2917](https://github.com/GoogleContainerTools/skaffold/pull/2917). + Finally, please use our best judgement when submitting pull requests.