cilium / design-cfps

Repo to store Cilium CFP design docs
Apache License 2.0
28 stars 25 forks source link

Should we merge PRs when they are not "Implementable"? #48

Open joestringer opened 1 month ago

joestringer commented 1 month ago

We've recently clarified status for CFPs (See #42).

While attempting to apply this standard to existing proposals, I noticed that there may be interest in merging CFPs when they are not considered "Implementable", but the current process largely assumes that this is the minimum bar to merge.

More specifically, there are a couple of cases I'm considering:

Dormant Status

These may require subsequent discussion to identify and resolve key questions prior to marking the CFP as "Implementable". I've already merged #7 under this category, and I think #36 also applies under this category. The status text does not currently really describe one way or another whether such PRs should be merged, but it seems like a reasonable way to "close the loop" on an open PR. The alternative I see would be to just close dormant CFP proposals, and if we wish to adopt them in future then someone would have to resurrect the PR afresh.

Draft Status

The current text prescribes that one of the properties of a draft is that it's not merged. It also describes "Implementable" PRs as being merged. This means that a specific idea needs to be significantly matured before it is merged into the repository. I noticed in #32 that contributors have been managing review discussions in their own public repositories in order to more easily iterate on the discussion. As a pattern, this can work, but it also draws the discussion away from the central official repositories, making them generally less visible to the overall community.

Furthermore when I reviewed the above PR, I had dozens of resulting comments. With so much activity, the GitHub UI tends to begin to inhibit healthy conversation by being slow, making it hard to find comments, and so on. This makes me wonder whether we should redefine the "Draft" status such that we could merge proposals in draft status as long is the core idea has high level agreemeent. We could set the culture to expect "TODOs" to be documented in a draft CFP before merge. Then, before the proposal can graduate to to "Implementable" we would need to ensure that key questions have reasonable solutions and have no major flaws and no significant lack of consensus.

Overall with this proposal, if we had community members working closely together, I could imagine staging the development of a CFP where an initial version just describes the summary and goals to establish high level consensus on the core idea, then subsequent PRs could develop the idea, each of which could undergo some degree of review to merge, then as key ideas/questions/impacts are introduced, even those could be separately committed, and only once the CFP gets to a point where there is rough consensus and committers believe it has been sufficiently explored, then a subsequent PR could move it to "Implementable". While I don't know for sure whether contributors are looking for this degree of incremental progress on CFPs given that CFPs are often written proposed by a single individual, I would be open to exploring mechanisms like this for more incremental progress.

joestringer commented 1 month ago

@xmulligan @youngnick @bowei @robscott may be interested to provide input on this. Probably the change to wording / process is shorter than the description of the idea above :sweat_smile: .

xmulligan commented 1 month ago

One idea could be to add something like an Idea status.

Can be used as an initial version that describes the summary and goals to establish high level consensus on the core idea. Subsequent PRs should develop the idea to address key ideas/questions/impacts as they are introduced. When there is rough consensus and committers believe the idea has been sufficiently explored, a subsequent PR should move it to Implementable.

xmulligan commented 1 month ago

I would maybe ask how long something would sit in this "idea" stage before it would go dormant, but maybe that isn't a big concern if it is already documented.

robscott commented 1 month ago

@xmulligan @youngnick @bowei @robscott may be interested to provide input on this. Probably the change to wording / process is shorter than the description of the idea above 😅 .

Thanks for looping me in to the conversation! While I think it can be valuable to merge proposals before they're "implementable", I think it's important to ensure the following first:

  1. Project maintainers are broadly supportive of the overall idea and consider it in scope for the project if unresolved issues can be worked out. (Merging without this leads to false hope and wasted efforts)
  2. Unresolved items have been clearly identified and are marked with some kind of standard keyword - Kubernetes uses UNRESOLVED for this. Ideally these blocks should provide a summary of any existing discussion along with links to relevant comment threads or docs.

The second point is particularly useful for follow ups. It helps separate the problem into clear follow up PRs that can each tackle a subset of unresolved problems, and it helps reviewers because otherwise it's very difficult to keep track of which parts of a proposal aren't quite settled yet.

bowei commented 1 month ago

+1 to Rob's comments.

youngnick commented 1 month ago

We have the Idea state in Gateway API, but there we call it Provisional. Same idea though - with all of @robscott's comments applying.

joestringer commented 1 month ago

Provisional state sounds good :+1: I agree that the differentiator between draft to provisional would be that there is a general sense of support for the idea that it is within scope and should be done. The "Unresolved" process also sounds like it would help to resolve my concerns about how we track open discussion items in a provisionally-accepted CFP.

joestringer commented 1 month ago

I'm idly wondering whether "dormant" and this sense of "provisional" state have more overlap than I thought. But I'm also fine to just leave dormant there for now and say that the next step after dormant is provisional - ie a dormant CFP has fewer guarantees about alignment than provisional. At least with provisional I'd expect that there is active discussion about the idea and someone planning to address the feedback. The current cases for "dormant" seem to be more like a good idea that needs someone to own it.