Skip to content

Revisit "Contributing Guide" #1952

@jpkrohling

Description

@jpkrohling

The Contributing Guide recommends that new components should be split into 3 PRs or more in order to get expedited reviews. However, this isn't improving the delay in reviewing PRs: the only PR that I submitted following this process took 10 days between the first PR getting merged and the last:

In addition to not being a better process, it makes contributing really hard, as it requires to either:

  1. plan ahead and know with relative certainty how the component is going to look like in the end; or
  2. implement the component as a working unit and later on split it into several PRs

The problem with the first is that we rarely know how it will finally look like: development is an iterative process, followed by successive refactoring sessions. The first and last commits might be predictable, but if anything in the implementation is bigger than 500 LoC, it should be split, and the final implementation is rarely predictable.

The problem with the second is that any changes required/suggested during the review does impact every PR that was made based on it. The practical effect is that I ended up trying to avoid changing my PRs to address review comments, opening a tracker issue instead to capture the non-blocking review changes.

In addition to the problems above, splitting a component/feature into several PRs may cause a component to be partially released: v0.11.0 has only the first commit out of the four linked above.

As a reviewer, I'm also concerned that I'll not get the full picture for a component just by looking at small PRs. I'm also not quite sure I'd benefit from reviewing several PRs instead of one: I typically know what to expect in a factory.go, config.go and other common files for a component. It's easy to review them, even when they are part of a bigger PR. But I'm sure other reviewers might disagree here, hence the current recommendation.

My proposal is to still recommend people to follow this structure in terms of commits that compose a PR instead of having multiple individual PRs. Example: https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/1231/commits . This PR has 3000+ LoC, but 16 commits, each one easy to be reviewed individually if the reviewer so wishes. Other reviewers can checkout the code locally and use a local code review tool to analyze it as a whole. A final benefit is that this single PR still results in a fully working unit providing the full picture for the component and without the risk to getting only partially merged.

Metadata

Metadata

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions