Update readme and template to reflect actual process being followed #275

dfawley · 2021-11-16T23:58:59Z

No description provided.

markdroth

This looks good! My comments are fairly minor.

markdroth · 2021-11-17T17:16:10Z

README.md

+## Updating proposals
+When updating a proposal, the PR description should be named as follows:
+
+    $CategoryName## update: <description of change>``


Please remove trailing back-ticks.

markdroth · 2021-11-17T17:18:16Z

README.md

+- ``Gnnnn`` - Protocol level changes.
+
+## Updating proposals
+When updating a proposal, the PR description should be named as follows:


Maybe we should say something about when and why we would update an existing gRFC instead of creating a new one? For example, maybe something like "Sometimes, after a gRFC is approved, small changes are needed to the design. In these cases, it is often simpler to modify the existing gRFC rather than create a new one."

Done (with slightly different wording).

markdroth · 2021-11-17T17:20:29Z

README.md

-Commits must not be squashed; the commit history serves as a log of changes
-made to the proposal.
+the APPROVER will approve the PR on GitHub. The PR will then be merged by either
+the OWNER, if possible, or the APPROVER otherwise.


Should we maybe explicitly say something here like "A proposal being merged indicates that it has been approved", just to make the state clear?

It seems pretty clear from the preceding sentence to me... I added a following sentence to remove any doubt. PTAL.

markdroth · 2021-11-17T17:21:11Z

GRFC-TEMPLATE.md

@@ -2,21 +2,19 @@ Title
 ----
 * Author(s): [Author Name, Co-Author Name ...]
 * Approver: a11r
-* Status: {Draft, In Review, Ready for Implementation, Implemented}


I'm thinking that we should probably go through the existing gRFCs and remove the "Status:" field, since it's pretty useless and misleading. But we can do that in a separate PR.

Agreed. I was thinking of the same, but maybe we should wait a month or two, since there are PRs pending with Status fields in them as well.

dfawley

Thanks for the comments. PTAL

dfawley · 2021-11-18T22:45:59Z

GRFC-TEMPLATE.md

@@ -2,21 +2,19 @@ Title
 ----
 * Author(s): [Author Name, Co-Author Name ...]
 * Approver: a11r
-* Status: {Draft, In Review, Ready for Implementation, Implemented}


Agreed. I was thinking of the same, but maybe we should wait a month or two, since there are PRs pending with Status fields in them as well.

dfawley · 2021-11-18T22:51:41Z

README.md

-Commits must not be squashed; the commit history serves as a log of changes
-made to the proposal.
+the APPROVER will approve the PR on GitHub. The PR will then be merged by either
+the OWNER, if possible, or the APPROVER otherwise.


It seems pretty clear from the preceding sentence to me... I added a following sentence to remove any doubt. PTAL.

dfawley · 2021-11-18T22:54:31Z

README.md

+## Updating proposals
+When updating a proposal, the PR description should be named as follows:
+
+    $CategoryName## update: <description of change>``


dfawley · 2021-11-18T22:56:42Z

README.md

+- ``Gnnnn`` - Protocol level changes.
+
+## Updating proposals
+When updating a proposal, the PR description should be named as follows:


Done (with slightly different wording).

GRFC-TEMPLATE.md

ejona86 · 2021-11-18T23:38:13Z

README.md

+
+## Updating proposals
+
+Sometimes small changes are needed to approved proposals.  Rather than create


I do think we should have some hints for what "small" is.

Sometimes small changes are needed to approved proposals. For example, editorial fixes, clarifying text, and detailed fixes noticed during implementation or usage. Rather than create new proposals for such changes, it is often better to revise the existing one.

Honestly I think anything that's not considered a "major" change should be a modification of a gRFC instead of a new one. A major change would be like an entirely different API or a wholly different mechanism for solving the same problem. "Minor" as described below, IMO, is for editorial fixes and clarifying text, which is allowed to be approved without the 10 day waiting period.

This is the original README.md text, for reference:

Once it is approved for submission by the arbiter, it goes into the Final state. Only minor changes are allowed (what qualifies as minor is left to the APPROVER).

If a proposal needs to be revisited, it can be moved back to the Draft or In Review state. This can happen if issues are discovered during implementation. At which point, the review process as described above must be followed.

Oh, so that's where you got that language... I think then we should harken back to the original language.

Only minor updates typically occur after a proposal is approved (and merged?). Minor updates do not require a comment period. What qualifies as minor is left to the APPROVER. If an update is needed and is not minor, then the process's 10 day comment period must be observed. This can happen if issues are discovered during implementation.

When updating a proposal, the PR description should be named as follows:
$CategoryName## update:

This is potentially more restrictive, but it also doesn't change what updates can occur and how compared to the existing text.

Only minor updates typically occur after a proposal is approved

Sorry, but I don't like your wording. "Only minor updates typically occur" -- to say "only X is typical" is unusual and unnecessary. The goal of the original process here was:

"Minor" changes to a gRFC are allowed with no 10-day discussion period.

Bigger changes to a gRFC are allowed with the 10-day discussion period.

I believe I expressed this adequately with my wording. I've made a small change here to move stuff / simplify. LMK if you disagree.

I disagree with your interpretation of the original text. I think the "This can happen if issues are discovered during implementation" part is important, as it provides a scope/context of the type of change we're talking about. We're not talking about redesigning old gRFCs 5 years after it was implemented because we no longer like that approach. Such things should be a new gRFC. Your proposed text encourages changing existing gRFCs, with no real limit on what sort of changes we'd expect to make.

If a gRFC has been implemented we probably aren't going to be materially modifying it in place except possibly to correct a problem discovered later or add a necessary, small tweak. I take the "This can happen if" as an example, not to mean "This can only happen if". IMO we should leave it up to our judgement on a case-by-case basis whether it's better to update a gRFC or create a new one. That's realistically already what we are doing, and I don't see this minor wording change (done to remove now-unnecessary clauses with the removal of the "Status" field) affecting anything.

@markdroth, thoughts?

I take the "This can happen if" as an example, not to mean "This can only happen if".

I agree. But the example does provide a guide. If someone wanted to do a huge change we could say, "that's a bigger change than we'd expect for an approved gRFC" and they wouldn't be surprised, because you can see how far of a departure that huge change is from the example. That is gone in the new language, and the new language encourages making the change to the existing gRFC without any limit, implied or explicit.

I can see both sides of this. I agree with Doug that we should ultimately leave the decision to the approver. But I also agree with Eric that we don't want to leave this completely open-ended; I think we do want to make it clear that non-minor changes to an existing gRFC are discouraged.

I will note that we do have one example where we made significant changes to a gRFC after it was already implemented, which was the modifications to A51 in #345. In hindsight, this really should have been done as a separate gRFC; I was the one who suggested modifying the existing gRFC, since I thought it would be a fairly simple change, but it turned out to be more complex than I expected. I think that having written guidance discouraging that might have prompted someone to object before that PR got merged.

In general, my feeling is that any modification to an existing gRFC should be minor enough that it doesn't require the 10-day comment period. I think that in most cases, if we're considering needing the 10-day comment period, that's probably a signal that we should be writing a new gRFC rather than modifying an existing one. But I don't want to be prescriptive about this -- I do think we want to allow the approver to use their judgement.

I suggest the following wording:

"""
Sometimes changes are needed to already-merged proposals, such as when we discover problems during implementation. Rather than create new proposals for such changes, it is often better to revise the existing one. However, this approach may not be appropriate for larger changes, especially after the design has already been implemented in most languages.

To make changes to an already-merged proposal, a PR may be approved and merged at the OWNER's and APPROVER's convenience. Note that the APPROVER will decide whether this kind of post-merge change is appropriate. The approver will also determine whether the 10-day comment period is necessary, based on how significant the change is.
"""

Seems fine to me. Updated.

markdroth

Looks great! Let's make sure Eric is happy with it, and then get it merged.

Update readme and template to reflect actual process being followed

4dfcb16

dfawley requested review from ejona86, ctiller and markdroth November 16, 2021 23:59

dfawley assigned ejona86, ctiller and markdroth Nov 16, 2021

markdroth reviewed Nov 17, 2021

View reviewed changes

review comments

48eb2e5

dfawley commented Nov 18, 2021

View reviewed changes

ejona86 reviewed Nov 18, 2021

View reviewed changes

dfawley added 2 commits November 22, 2021 09:38

re-add discussion link; clarify

726cef5

Update README.md

faecf17

markdroth approved these changes Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update readme and template to reflect actual process being followed #275

Update readme and template to reflect actual process being followed #275

dfawley commented Nov 16, 2021

markdroth left a comment

markdroth Nov 17, 2021

dfawley Nov 18, 2021

markdroth Nov 17, 2021

dfawley Nov 18, 2021

markdroth Nov 17, 2021

dfawley Nov 18, 2021

markdroth Nov 17, 2021

dfawley Nov 18, 2021

dfawley left a comment

dfawley Nov 18, 2021

dfawley Nov 18, 2021

dfawley Nov 18, 2021

dfawley Nov 18, 2021

ejona86 Nov 18, 2021

dfawley Nov 19, 2021

dfawley Nov 19, 2021

ejona86 Nov 19, 2021

dfawley Nov 22, 2021

ejona86 Dec 8, 2021

dfawley Dec 8, 2021

ejona86 Dec 16, 2021

markdroth Dec 13, 2024

dfawley Dec 13, 2024

markdroth left a comment


		## Updating proposals

		Sometimes small changes are needed to approved proposals. Rather than create

Update readme and template to reflect actual process being followed #275

Are you sure you want to change the base?

Update readme and template to reflect actual process being followed #275

Conversation

dfawley commented Nov 16, 2021

markdroth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment