Skip to content

streamline CFP approval and tracking process#37

Closed
katiestruthers wants to merge 2 commits intocilium:mainfrom
katiestruthers:pr/improve-cfp-process
Closed

streamline CFP approval and tracking process#37
katiestruthers wants to merge 2 commits intocilium:mainfrom
katiestruthers:pr/improve-cfp-process

Conversation

@katiestruthers
Copy link
Copy Markdown

@katiestruthers katiestruthers commented May 13, 2024

Inspired by the Gateway Enhancement Proposal (GEP) docs.

I tried to combine @joestringer's comment in #4 with the three proposed states in that same issue, resulting in 6 possible statuses (I renamed 'State' to 'Status' just to fit the default for GitHub Projects).Experimental and Stable could use some stronger definitions, but I think the other four make sense.

These statuses will be listed in the Cilium Feature Proposals (CFP) GitHub Project that will be added to this repository. Workflows are set to automatically set Provisional (when a new PR is opened), Implementable (when a PR is merged), and Withdrawn (when a PR is closed), but the other three statuses will have to be manually set. The SIGs will also have to be manually set.

I have also opened a PR to update the list of SIGs, so those links I added will be more useful soon.

Copy link
Copy Markdown
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this. I think that having a set of statuses that contributors can use to label their CFPs is a helpful step in establishing the process. Ultimately I think that these will just have to be tested against reality to prove the usefulness for different stages and to establish precedence about how the community treats CFPs with different statuses, but until then I've provided some minimal feedback.

Given that it's relatively new to the Cilium community to track & review designs using a git repository, I think we may struggle a little bit to clearly define how the overall process should work to balance velocity and scrutiny, so some of my feedback below may not be directly relevant to the aim of this PR (defining status) vs. understanding how this status fits into the wider process. Bear with me as I try to figure this stuff out with you all :)

I dropped a couple of minor wording nits below as well on how to improve some of the wording.

Cilium Github organization.
Cilium Github organization.

All current CFPs are listed in the [Cilium Feature Proposals (CFPs) project]() found in this repository.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: What information does this communicate that the previous paragraph doesn't communicate? Should it be skipped or combined with the previous paragraph?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

@joestringer joestringer May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, I missed the PR description when I first read the PR 🤦

These statuses will be listed in the Cilium Feature Proposals (CFP) GitHub Project that will be added to this repository. Workflows are set to automatically set Provisional (when a new PR is opened), Implementable (when a PR is merged), and Withdrawn (when a PR is closed), but the other three statuses will have to be manually set. The SIGs will also have to be manually set.

How will we manage these? If this is automated somehow, great. If not, then I think this is creating more busywork that is likely to be neglected by the Cilium project unless you find someone specific who will volunteer to keep the CFP projects up to date. Almost every time we've tried to use projects or milestones etc. in the Cilium project, there's one person who pays attention to them for a few weeks, then they lose interest and the project gets stale and outdated. A big reason I see for this is that the benefits of managing this "virtual paperwork" are not high enough to make it part of our regular working process.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is something that SIG leads see value in and would be happy to help with, then I would encourage setting up something like that. On the other hand, if we don't have buy-in from anyone who would actively engage with this process, then I think it's the process that needs to change to fit the people, not the people changing to fit the process.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll CC in specifically @marseel , @hemanthmalla (on behalf of @cilium/sig-scalability) and @squeed (on behalf of @cilium/sig-policy) in case they have opinions about how to track CFPs within specific SIGs and whether sharing a project to track these things would be helpful for them.

Comment on lines +40 to 42
You are encouraged to share your PR in the [appropriate SIG slack channel](https://docs.cilium.io/en/stable/community/community/#all-sigs) to speed up the approval process. If you want to bring further attention to your design, you may want to
raise the design during the [weekly community call](https://docs.cilium.io/en/v1.13/community/community/#id1)
and on the [#development channel in Slack](https://cilium.slack.com/archives/C2B917YHE).
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link here helps 👍

nit: To simplify the wording and make it accessible to readers from a variety of backgrounds, I would suggest to use more straightforward imperative wording, such as:

Suggested change
You are encouraged to share your PR in the [appropriate SIG slack channel](https://docs.cilium.io/en/stable/community/community/#all-sigs) to speed up the approval process. If you want to bring further attention to your design, you may want to
raise the design during the [weekly community call](https://docs.cilium.io/en/v1.13/community/community/#id1)
and on the [#development channel in Slack](https://cilium.slack.com/archives/C2B917YHE).
Promote awareness about your CFP in the community:
- Share it on the [#development channel in Slack](https://cilium.slack.com/archives/C2B917YHE) or [one of the more specific SIG channels](https://docs.cilium.io/en/v1.13/community/community/#id1).
- Raise the design for discussion during the [weekly community call](https://docs.cilium.io/en/v1.13/community/community/#id1).

sidenote regarding the above feedback, while community members "are encouraged" and they may "want to bring further attention...", this wording is imprecise, and we've adopted a style guide for better communication since the previous version of this doc was written.


1. ``Provisional``: All new PRs are given this status. This CFP's design has not yet been approved.
2. ``Implementable``: Approved PRs that are merged are given this status. Their CFP design has been approved, and now the work can be started to implement them.
3. ``Experimental``: The CFP has been implemented and is now in a 'Beta' state.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it experimental, or is it beta? These words have different connotations ranging from "Use at your own risk" to "Please run this in production and report your findings".

I recognize the need for a category like this between "Implementable" and "Stable", though I'm not familiar enough with process or how the process impacts human behaviour in order to have a strong opinion on how to portray a proposal in this status.

I can see benefits in leaning it towards wording like "experimental" as a way to incentivize contributors to work on moving the CFP towards "Stable" state, though I could equally see contributors simply ignoring the CFP process after it becomes "Implementable" and completely focusing on stabilizing the functionality to the point where it's production-ready.

I'll CC @youngnick in here in case he has some opinion about how a process like this should look, as I think he has more experience than me on this.

Copy link
Copy Markdown
Member

@joestringer joestringer May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related: For the Cilium project, this is likely to differ from other communities like Gateway API, because these designs in Cilium are targeted for typically one implementation in Cilium (one of the repos under this organization), and we have additional expectations about testing, docs, etc for that implementation as we accept changes. On the other hand when the entire project is a standard like Gateway API or Network Policy API in upstream k8s sig-network, then some of those requirements may end up in the design repo.

1. ``Provisional``: All new PRs are given this status. This CFP's design has not yet been approved.
2. ``Implementable``: Approved PRs that are merged are given this status. Their CFP design has been approved, and now the work can be started to implement them.
3. ``Experimental``: The CFP has been implemented and is now in a 'Beta' state.
4. ``Stable``: The CFP has been implemented, rigororously tested, and is now in a 'GA' state.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess related to the previous comment, we have the state of the CFP then the state of the feature. Each of these lines seems to describe the state of the CFP as tracking a specific state of the feature, though these terms like 'beta' or 'GA' are not currently defined anywhere, so it's a little hard to understand what those mean.

Is the rigorous testing intended for the design or the implementation?

2. ``Implementable``: Approved PRs that are merged are given this status. Their CFP design has been approved, and now the work can be started to implement them.
3. ``Experimental``: The CFP has been implemented and is now in a 'Beta' state.
4. ``Stable``: The CFP has been implemented, rigororously tested, and is now in a 'GA' state.
5. ``Complete``: All implementation work is complete and has been merged.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this "Complete" state different from "Stable"?

4. ``Stable``: The CFP has been implemented, rigororously tested, and is now in a 'GA' state.
5. ``Complete``: All implementation work is complete and has been merged.

At any time during the process, the CFP can be given the status of ``Withdrawn`` if it is no longer thought to be viable, or if it is not wanted to be pursued for any other reason. No newline at end of file
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To help inform the thought process, I have a PR (#36) that sort-of fits this category. I labelled it "Dormant" for now, because in this case the design review resulted in "Changes Requested", but I do not currently have the cycles to pursue the change further. The core idea is still worthwhile, but it hasn't proven critical enough to solve just yet. The idea isn't exactly "Withdrawn", but the specific design is practically speaking "Withdrawn". I'm not sure whether I would consider that one to be under "Provisional" or "Withdrawn" given that the primary blocker on moving it forward is just interest from a contributor to resolve the outstanding feedback and push it forward.

@xmulligan
Copy link
Copy Markdown
Member

Katie originally put together a Github project to automate some of this Cilium Feature Proposals (CFPs) (view). However, thinking about this again, since there hasn't been a lot more discussion about what the best process is, it might be a bit of premature optimization.

I'll pull out some of the wording improvements and we can refer back to this discussion if/when we want to revisit this in the future. For now, I think we should just focus building out the operations of the SIGs and review this again if we think we need a better process around CFPs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants