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

RFC: add status field #177

Closed
wants to merge 5 commits into from
Closed

Conversation

aemengo
Copy link
Contributor

@aemengo aemengo commented Jul 14, 2021

# Unresolved Questions
[unresolved-questions]: #unresolved-questions

- Should **"Implemented"** be one of the accepted values?
Copy link
Member

Choose a reason for hiding this comment

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

Also (perhaps) "Superseded" "Retracted"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are there currently RFCs that we'd like to apply "Superseded" or "Retracted" to? I'd like to defer adding new values till when the need arises.

Copy link
Member

@ekcasey ekcasey Jul 28, 2021

Choose a reason for hiding this comment

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

I think there are a handful of superseded RFCs. For example the Supersedes section of RFC 0044 implies that some part of RFC 0022 is superseded. Although, tbh it's not clear at a glance which parts (I suspect the entire RFC is not superseded). This might be another case where notes would help!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense to me!

@aemengo aemengo marked this pull request as ready for review July 20, 2021 11:50
@aemengo aemengo requested a review from a team as a code owner July 20, 2021 11:50
text/0000-add-state-field.md Outdated Show resolved Hide resolved
# Motivation
[motivation]: #motivation

This RFC formally introduces the **"On Hold"** state for an RFC, in a way that's clear to the community.
Copy link
Contributor

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 adding an explanation of why this field is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm partial to Stephen's thoughts here, saying:

I suggest we require individual RFCs to specify why they might be in the On Hold state, and not define a reason for On Hold in this RFC.

#177 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

When deemed "ready", a team member will propose a "motion for final comment period (FCP)" along with a disposition of the outcome (merge, close, or postpone).

Also, there's a postpone disposition today that we don't take advantage of. I wonder if that's something we can use here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@aemengo, my comment is on the motivation section:
This RFC formally introduces the **"On Hold"** status for an RFC, in a way that's clear to the community.
What do you mean by "in a way that's clear to the community"?
I think it's worth adding an explanation on why do we want this RFC.
Maybe giving an example or two may help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yaelharel

What do you mean by "in a way that's clear to the community"?

I suppose I meant, in a way that is clear to anyone reading the RFC. As opposed to, through word-of-mouth.

Maybe giving an example or two may help?

I don't think adding a Status: to RFCs is a novel idea. We can see them as part of other RFC templates, for instance Tekton.

@aemengo
Copy link
Contributor Author

aemengo commented Jul 22, 2021

Addressing comments from Working Group (07/22/2021):

I have a slight preference for Status: over State:

Fixed in latest commit.

Having the full list of acceptable values, demarcated by a |, could get a bit long.

I'd prefer not to pre-optimize here. So far, we've only expressed need for the "On Hold" status. When we start adding more, we can reconsider the format.

edit: Fixed in latest commit.

Perhaps we can have the template say Status: Draft and then replace it when "Approved".

Similar to above, I prefer seeing the list of acceptable values. Just so things can't get out sync. Thus we can remove the values that are implied.

Since this appears to be a not-strong, weakly-held opinion, we can defer to the OP's preference here. Moreover, this isn't hard to change if it doesn't work out.

edit: Fixed in latest commit.

0000-template.md Outdated Show resolved Hide resolved
0000-template.md Outdated Show resolved Hide resolved
@aemengo aemengo force-pushed the add-state-field branch 2 times, most recently from 8d09f74 to 0f50d69 Compare July 22, 2021 21:15
@aemengo aemengo changed the title RFC: add state field RFC: add status field Jul 22, 2021
@aemengo aemengo force-pushed the add-state-field branch 2 times, most recently from 6cdda67 to e4e9633 Compare July 22, 2021 21:23
```
- Status: On Hold
```

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Pull requests that change the status of a previously accepted RFC to "On Hold", should add a Section titled `Notes` to the top of the RFC containing an explanation for the change of status.
Example:
\```
# Notes
## On Hold
Since the approval of this RFC additional concerns X, Y, and Z have been raised. Solving problem Foo is still a priority for the project but the core team has decided to place this RFC on hold pending further discussion of alternative solutions. If you are interested in problem Foo place participate in the discussion on RFC #XXXX.
\```
If at a later date the core team or responsible sub team wishes to move the RFC from "On Hold" back to "Approved", this should be done via PR and another note should be added, describing the resolution of the discussion and the rationale for proceeding with the original RFC.

Copy link
Member

Choose a reason for hiding this comment

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

I feel if we want to move a RFC back off of hold there should have a superseding RFC (in most cases).

- Perhaps, but then what's the process of keeping the _Implemented_ status in sync?

- How do we retroactively add a **Status:** field to prior RFCs?
- Manually?
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think manually is fine.

- Name: Add Status to RFC Metadata
- Start Date: 2021-07-14
- Author(s): @aemengo, @ekcasey
- Status: Draft <!-- Acceptable values: Approved, On Hold, Draft -->
Copy link
Member

Choose a reason for hiding this comment

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

Should we add "Superseded" to the list of statuses? An RFC PR that includes RFCs in the "Supersedes" section should also changed the status of all referenced RFCs to "Superseded".

Suggested change
- Status: Draft <!-- Acceptable values: Approved, On Hold, Draft -->
- Status: Draft <!-- Acceptable values: Approved, Draft, On Hold, Superseded -->

(I also alphabetized the statuses in this suggestion b/c I couldn't see the logic in the original ordering)

Copy link
Member

Choose a reason for hiding this comment

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

Is there value of having a "Draft" status? We don't have reject here.

Copy link
Member

Choose a reason for hiding this comment

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

Is there value of having a "Draft" status?

If someone links you to a readable version of a not-yet-merged RFC, you can see that it is still a draft.

Copy link
Member

Choose a reason for hiding this comment

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

Should we include closed/postponed as well then? Is there a distinction between "status" and "disposition"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally called this out as an anti-goal; trying to show all the status that could be inferred by the PR process. Don't know how you still feel about closed/postponed

Copy link
Member

Choose a reason for hiding this comment

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

Should we include closed/postponed as well then?

Practically speaking, I don't think this would be actionable. We don't have the power to update the status field in someone's fork when an RFC is closed or postponed, and nagging people to update it doesn't feel worth it from a cost/benefit perspective.

@hone I wanted Draft because it seemed like an informative default for in progress RFCs. But I assumed it would be uncontroversial. I am happy to leave status blank until we merge an RFC if you feel strongly about it.

[unresolved-questions]: #unresolved-questions

- Should **"Implemented"** be one of the accepted values?
- Perhaps, but then what's the process of keeping the _Implemented_ status in sync?
Copy link
Member

Choose a reason for hiding this comment

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

What's the goal we're trying to solve with the "Status" field? I think answering this would help answer this question.

Copy link
Member

Choose a reason for hiding this comment

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

Based on the motivation of this RFC, it seems as though it's to clearly portray the status of any given RFC. If that's the case I would argue that knowing whether something is "Implemented" or "Implementable" is a helpful distinction.

As a prior art: Tekton TEPs use these statuses: https://github.com/tektoncd/community/tree/main/teps#teps

- Manually?

- How do we validate the [#meta](#meta) section of prior RFCs?
- Perhaps we make a [../validate-rfcs.sh](../validate-rfcs.sh) script that lints the **#meta** section of all rfcs?
Copy link
Member

Choose a reason for hiding this comment

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

maybe we can leverage Javier's automation.

- Name: Add Status to RFC Metadata
- Start Date: 2021-07-14
- Author(s): @aemengo, @ekcasey
- Status: Draft <!-- Acceptable values: Approved, On Hold, Draft -->
Copy link
Member

@jromero jromero Jul 28, 2021

Choose a reason for hiding this comment

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

Suggested change
- Status: Draft <!-- Acceptable values: Approved, On Hold, Draft -->
- Status: Draft <!-- Acceptable values: Draft, Implementable, Implemented, On Hold, Superseded -->

We've discussed using Implementable and Implemented in other conversations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ekcasey @natalieparellano @hone

I'm sorry for not being present when these values were posited. Can I please just confirm that the following acceptable values are what's preferred?

Draft, Implementable, Implemented, On Hold, Superseded

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea in general of indicating to users whether an RFC has been implemented, but I anticipate amibiguity regarding the definition of "implemented" as a status field. For example, image a change that adds a feature to the platform API is it implemented when the spec is released? when the lifecycle is released? when pack exposes it to users?

Given the ambiguity I suspect we would want something more detailed than a binary implementable/implemented distinction, like an implementation history section (example https://github.com/kubernetes/enhancements/tree/master/keps/sig-cluster-lifecycle/kubeadm/970-kubeadm-config#implementation-history)?

If this is contentious or complicated though I think we could break this out into a different RFC and go ahead with a simple Approved status for now.

Copy link
Member

Choose a reason for hiding this comment

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

@ekcasey, I agree. I can see that we'd want to define Implemented. For the sake of not ballooning this RFC, I'm more that okay with only moving forward with Approved.

Copy link
Member

Choose a reason for hiding this comment

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

Does that mean that we agree on a total of:

Draft, Approved, On Hold, Superseded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I agree.

0000-template.md Outdated Show resolved Hide resolved
merge-rfc.sh Outdated Show resolved Hide resolved
@aemengo aemengo force-pushed the add-state-field branch 2 times, most recently from 2bd662b to 688c2ed Compare August 20, 2021 15:48
@hone
Copy link
Member

hone commented Sep 8, 2021

Moving to FCP to close on Sep 15.

@buildpack-bot
Copy link
Member

buildpack-bot commented Sep 8, 2021

Maintainers,

As you review this RFC please queue up issues to be created using the following commands:

/queue-issue <repo> "<title>" [labels]...
/unqueue-issue <uid>

Issues

  • ⬜️ e91988 - buildpacks/rfcs "Back fill status fields"

Anthony Emengo and others added 5 commits September 13, 2021 22:47
Signed-off-by: Anthony Emengo <aemengo@vmware.com>
Co-authored-by: Stephen Levine <stephen.levine@gmail.com>
Signed-off-by: Anthony Emengo <aemengo@vmware.com>
Signed-off-by: Anthony Emengo <aemengo@vmware.com>
Co-authored-by: Sambhav Kothari <sambhavs.email@gmail.com>
Signed-off-by: Anthony Emengo <aemengo@vmware.com>
Signed-off-by: Anthony Emengo <aemengo@vmware.com>
@hone
Copy link
Member

hone commented Sep 22, 2021

/queue-issue buildpacks/rfcs "Back fill status fields"

@hone
Copy link
Member

hone commented Sep 22, 2021

Not sure why this didn't autoclose, but was merged as RFC 0094!

@hone hone closed this Sep 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet