github.com/GoogleContainerTools/skaffold/v2@v2.13.2/docs-v2/community/small-prs.md (about)

     1  # Pull Request Guidelines
     2  
     3  Writing good pull requests increases development velocity and minimizes frustration.
     4   
     5  Small pull request will be merged quicker.
     6  
     7  Having big pull requests that bounce back and forth between developers
     8  and reviewers can slow progress down significantly, causing developers to waste a 
     9  lot of time dealing with merge conflicts.
    10  
    11  Big pull requests also increase the risk of breaking things; since changes are so big, 
    12   - it’s hard to truly understand all the changes and 
    13   - test them for regressions.
    14  
    15  In order to promote good PR quality, the skaffold team will push back
    16  against big PRs. See [0](https://github.com/GoogleContainerTools/skaffold/pull/2750#pullrequestreview-283621241), [1](https://github.com/GoogleContainerTools/skaffold/pull/2917#pullrequestreview-291415741)
    17  
    18  ## Breaking down Pull Requests
    19  
    20  ### Feature development.
    21  When proposing a new feature, please review our [Design Document Proposal](../design_proposals) to see if you should first create a `Design Proposal`.
    22  
    23  Adding a new command or skaffold config results in large number of changes due to
    24  all the boilerplate code required for adding a new command, generating docs etc.
    25  
    26  It makes sense to break down these changes into 2 categories:
    27  1. "Invisible changes": Incremental, small code changes 
    28       1. refactoring: either keep the functionality the same but refactoring the design or
    29       1. partial implementations: unit tested, new behavior, that is not yet accessible by the main control flow.
    30  2. "Visible changes": User affecting changes, for example
    31     1. Adding a new command which exercises the above code changes
    32     1. Adding config change and generating docs.
    33  
    34  #### Config Change Example   
    35  Below is an example of how you can break a config change into small individual PRs.
    36  
    37  A contributor wanted to support [custom build args to `kustomize` deployer](https://github.com/GoogleContainerTools/skaffold/issues/2488).
    38  This required skaffold to add the `deploy.kustomize.buildArgs` field and then plumb the arguments to the `kustomize` command.
    39  
    40  This small 10 line change results into 100+ lines of code along with test code.
    41  It would greatly simplify code review process if we break this change into 
    42  1. Coding logic and test coverage.
    43  2. User facing documentation.
    44  
    45  In this case, the contributor split this PR in 2 small changes
    46  1. Introduce a [place holder for a new config](https://github.com/GoogleContainerTools/skaffold/pull/2870).
    47     
    48     This PR highlights the logic changes and makes it easier for reviewers to review code logic and tests.
    49     
    50     Note: The code added in this PR does not get exercised other than in tests.
    51  2. [Add a field to skaffold config](https://github.com/GoogleContainerTools/skaffold/pull/2871) and pass it to the place holder.
    52     This PR makes it easier for reviewers to review the user facing config changes and make sure all the precautions like
    53     - good user documentation
    54     - deprecation policy if applicable
    55     - upgrade policy etc were followed. 
    56  
    57  #### Adding new functionality
    58  Below is an example on how you can introduce a new functionality to skaffold in smaller incremental PRs.
    59  
    60  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
    61  1. Add this feature to `kubectl`, `helm` and `kustomize` deployers supported by skaffold.
    62  2. support a new flag for `skaffold deploy` and `skaffold run` command
    63  3. may be add a new command to only render k8 manifests.
    64  
    65  Item 2 and 3 in the above list of changes, are user facing features and could be abstracted out.
    66  Item 1 can further be implemented incrementally by supporting this new feature for each of the deployer.
    67  
    68  To implement this feature, we 
    69  1. First added an [method to interface `deployer` with empty stub](https://github.com/GoogleContainerTools/skaffold/pull/2834).
    70     
    71     Note: The code added in this PR does not get exercised other than in tests.
    72  2. Then add render implementation for each [kubectl deployer](https://github.com/GoogleContainerTools/skaffold/pull/2943), kustomize and helm.
    73  3. Add a [render command](https://github.com/GoogleContainerTools/skaffold/pull/2942)
    74  
    75  Remember, its ok to land code which is not exercised. However please mention the follow up work
    76  in **Next PRs** section when creating a Pull Request [e.g. here.](https://github.com/GoogleContainerTools/skaffold/pull/2811)
    77  
    78  Implementing the full functionality sometimes might makes a lot of sense,
    79   - so that you can get feedback regarding the code while implementing it also
    80   - the maintainers can try it out and test it, get a feel for it. 
    81  
    82  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`.
    83  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. 
    84  See for example [#2917](https://github.com/GoogleContainerTools/skaffold/pull/2917).
    85  
    86  Finally, please use your best judgement when submitting pull requests, these rules might not always work for you - we would love to hear that!