Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add status to the README #42

Merged
merged 7 commits into from
Aug 9, 2024
Merged

add status to the README #42

merged 7 commits into from
Aug 9, 2024

Conversation

xmulligan
Copy link
Member

Following #4 #37 and #9 (comment)

I've taken a stab at outlining a status for the CFP lifecycle. On this first pass I've tried to keep it pretty simple. Unmerged CFP is draft, merged is implementable, if a CFP is updated status is updated, and if it hasn't moved forward or was just an idea it is dormant.

Practically speaking that means that any CFP that has one committer's approval is now implementable. Hopefully, this will make it more clear that once there is alignment on the CFP, we can merge it so they aren't stuck as open PRs forever. We can then updated a CFP later as we see fit.

Signed-off-by: Bill Mulligan <[email protected]>
Copy link
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.

Makes sense to me in principle. Couple of minor pieces of feedback:

  • Updated status is a bit unclear to me - I think it's similar to Implementable but just demonstrates that the design was revised since the first design? But still may not be fully implemented.

  • Might want an "implemented" status

  • I briefly considered whether "dormant" should be separate from "draft", and figured that yes it probably makes sense to keep both separately, to help communicate where there is active interest on the CFP.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: Joe Stringer <[email protected]>
Signed-off-by: Bill Mulligan <[email protected]>
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@joestringer joestringer requested a review from youngnick July 16, 2024 17:17
@joestringer
Copy link
Member

Oh I think Nick is actually OOO, so no need to block this PR on his response. I just figured he could potentially provide some good feedback.

Once you've revised the latest wording I can take a fresh look over the full text. We could also just advertise this a bit more broadly and set a deadline to merge. I think that after some minor editing this should be plenty good enough to start using and we can further amend the process from there as needed.

Copy link
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

LGTM with @joestringer's changes. I think keeping it simple for now is a great idea, the Gateway API process has evolved over time, but YAGNI applies to processes as much as code. 😊

README.md Outdated Show resolved Hide resolved
@xmulligan
Copy link
Member Author

Sorry just catching up after vacation too. I've slightly changed the wording to released so we aren't going between implementable and implemented. I think it is ready for another read through and I can bring it to the developers meeting too.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
Signed-off-by: Bill Mulligan <[email protected]>
@xmulligan
Copy link
Member Author

xmulligan commented Aug 1, 2024

@bowei Thanks for pointing out the GEP process. I've looked through it and added Declined as a status too. I'll also add my thoughts on why I didn't add the other statuses below

Completed matches Released

Standard and Experimental match more to feature maturity/stability. So far in the Cilium project history, we haven't really tracked feature maturity. At a very high level there is the Major Feature Status, however we currently don't have clear guidelines around them and trying to add this right now to the CFP process would seem like a major change to the project workflow and I would try to avoid that right now unless we have a clear idea about what we are trying to get out of it

Implementable, Provisional, Prototypingseem to roughly matchImplementable`. It is not quite clear to me the benefit of having these three different statuses to say we roughly agree on the idea, but haven't started all of the coding yet. Do you think we should have multiple statuses here?

@xmulligan xmulligan requested review from joestringer and bowei August 1, 2024 13:05
@youngnick
Copy link
Contributor

I think that it's better to start with less states, and add them if we need them later. I agree with adding Declined, but I think that what's there now is enough for a first iteration.

README.md Outdated
After the approval, the design can be merged. A merged design proposal
means the proposal is viable to be executed on, but not that there is a
100% chance it will be accepted.
For a CFP to be considered implementable, a Cilium committer needs to aprove it.
Copy link
Member

Choose a reason for hiding this comment

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

typo: aprove => approve

README.md Outdated Show resolved Hide resolved
README.md Outdated
upon as implementable. Dormant CFPs can be reactivated
at any time if there is interest in the project. The next step for a dormant CFP is to amend it to be Implementable.

5. Declined: This proposal was considered by the community but ultimately rejected.
Copy link
Member

Choose a reason for hiding this comment

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

Suggest add:

"The community may come back to the proposal in the future but a new CFP should be used. Rejected proposals are useful as documentation on why the given proposal did not make sense."

Copy link
Member

Choose a reason for hiding this comment

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

why the given proposal did not make sense.

How do we document this part?

If there's a PR and it's closed as not planned, I think the UX should be fairly clear. On the other hand, if the PR is merged into the repository, then the PR itself needs to be updated to encode this information. One of the things we've struggled with a bit as we start to establish the CFP process is that for the most part, developers don't want to document what they're designing. If you then decline their proposal and ask them to put additional effort into documenting why, then they're probably not all that motivated to comply. On the other hand, I guess as committers/maintainers we could take on that work, but in that case we also (a) need buy-in from those folks and (b) could benefit from an example workflow that describes what those folks should do in order to document the decision and why we feel it's important (helps with buy-in).

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated the wording to "can be" and leaving the implementation that Joe is discussing above to be worked out when we come to that bridge.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's worth noting that the default behaviour if we don't resolve that question will be either (a) to leave PRs that have been declined open, or to (b) just close them without merging. Either of these options takes minimal to no effort vs. figuring out the standard we expect in order to merge a declined CFP.

That said, I'm OK with this outcome and we can always improve later, I just want to set expectations that even if we agree in principle that merging declined CFPs is OK, unless we have a specific pattern to follow, we will not merge declined PRs.

README.md Outdated
The status of a CFP indicates its maturity. There are four different statuses
that a CFP can have:

1. Draft: A CFP in any form (Google doc, Github issue, ect.) that has not yet
Copy link
Member

Choose a reason for hiding this comment

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

See my comment above -- it would be good to mandate that a Github issue be opened to track progress from Draft state i.e. make it a MUST that there is an issue. This makes it very easy for the project to track all draft proposals that pending (e.g. for triage).

Copy link
Member

Choose a reason for hiding this comment

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

Agree. I think we already have this expectation at some level, because the number of the CFP comes from the issue number on the corresponding repository. Even if it's a stub we should just make that part of the standard workflow.

Copy link
Member

Choose a reason for hiding this comment

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

(Worth noting though that for any formal CFP there will be a PR in this repository, so tracking feels like it should be relatively straightforward by just listing the open PRs in this repository... but regardless of that I think this is worthwhile.)

Copy link
Member Author

Choose a reason for hiding this comment

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

updated the wording slightly to make Github issue MUST

@bowei
Copy link
Member

bowei commented Aug 2, 2024

Just so we don't forget, we may want to backpatch the existing CFPs with the status field for completeness...

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
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.

Pretty good, couple of minor bits of feedback on wordsmithing.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@xmulligan xmulligan requested a review from bowei August 8, 2024 02:08
@bowei
Copy link
Member

bowei commented Aug 8, 2024

LGTM

@xmulligan xmulligan merged commit 79e8aa0 into main Aug 9, 2024
1 check passed
@xmulligan xmulligan deleted the xmulligan-patch-1 branch August 9, 2024 09:28
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.

4 participants