-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Description
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:
- Authentication Helper 1/4 - Add configauth #1807
- Authentication processor 2/4 - Add auth context and interface #1808
- Authentication processor 3/4 - Add implementation #1809
- Authentication processor 4/4 - Add configauth to configgrpc #1810
In addition to not being a better process, it makes contributing really hard, as it requires to either:
- plan ahead and know with relative certainty how the component is going to look like in the end; or
- 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.