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: 0016 RFC Process #53

Merged
merged 7 commits into from
Jan 8, 2020
Merged

RFC: 0016 RFC Process #53

merged 7 commits into from
Jan 8, 2020

Conversation

MrArnoldPalmer
Copy link
Contributor

@MrArnoldPalmer MrArnoldPalmer commented Dec 18, 2019

Notes

These were put together using a combination of the React and Rust RFC process documents. Please suggest any additions or changes we think we should incorporate for our own process. As the readme says, the process is intentionally compromising as to allow for some organic evolution.

  • Still need good examples of "Prior Art". The react RFC template doesn't include this section but I'm partial to it so I've kept it in. I want to get the teams feedback on whether we think this is something we should include. Removed prior art for now in favor of a more "react" style document that is less procedural and a little more broad. We can revisit this later as I believe it may prove useful for things like spec definition.
  • I formatted these documents using prettier. I vote that we add a basic style guide, possibly even a package.json with tools/scripts we use to format and otherwise automatically verify the content of RFCs. This can be done separately if the team desires and could be useful if we write some automation of initial feedback using github actions or travis. Rust's RFC repo includes a styleguide and recommendations to use a formatting tool.
  • Rust RFC project has a bot that creates a gitbook with active RFCs https://rfcbot.rs/, something to consider maybe?
  • Should we define a pull request template and labels (e.g. "final comment period", "needs review", etc). PR and tracking issue templates are included.

Rendered


By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache-2.0 license

Adds readme with description of the RFC process and how to use it.

Adds `0000-template.md` to be used as a template for creating new RFCs.
Copy link
Contributor

@RomainMuller RomainMuller left a comment

Choose a reason for hiding this comment

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

There's a lot of very CDK-centric language here - and most of it is fine, but as we're also using this for jsii, I think we should be less specific in any places... For example, in the template, there is:

  • enough detail for somebody familiar with CDK to understand

  • the impact on teaching people how to use CDK

  • cost of migrating existing CDK applications


It's great you've dropped an Apache-2.0 license file. It lacks the corresponding NOTICE file that our standards demand is present... And it'd be dope to also drop the standard CODE_OF_CONDUCT.md file in there, too.

README.md Outdated Show resolved Hide resolved
README.md Outdated
Comment on lines 21 to 22
- Any change to existing APIs that would break existing code.
- The removal of existing features or public APIs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Well we shouldn't be doing this at all (breach of semver)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we change this to "deprecation of existing features or public APIs."? or just remove this line altogether?

I'm not sure how this breaks semver? It would just require a major version bump no?

README.md Outdated Show resolved Hide resolved
README.md Outdated
- Bugfixes for known issues.
- Additions only likely to be _noticed by_ other developers-of-CDK,
invisible to users-of-CDK.
- Additions of missing L1 or L2 constructs.
Copy link
Contributor

Choose a reason for hiding this comment

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

In the general case, yes...In certain specific cases, probably deserves an RFC. The case is for example for those complex services with an embedded DSL or document language (API Gateway, ECS, EKS, ...) where the CDK API would aim to simplify the user experience and that is effectively "interesting" (read: complex enough) API design.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. If the API design is non-trivial, it's probably worth

README.md Outdated Show resolved Hide resolved
LICENSE Show resolved Hide resolved
0000-template.md Outdated
Comment on lines 1 to 4
- Feature Name: (fill me in with a unique identifier, my-awesome-feature)
- Start Date: (fill me in with today's date, YYYY-MM-DD)
- RFC PR: (leave this empty)
- Related Issue: (leave this empty)
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. how about we make this into a front-matter (those render specially in GitHub, and I feel like it's a good fit)
    ---
    name: <unique identifier>
    date: <YYYY-MM-DD>
    pr: <pr-id-once-assigned>
    issue: <issue-id-once-assigned>
    ---
    
  2. I'd like to encourage people to cross-reference RFCs, like an RFC may be an extension of another, may replace another, etc...

0000-template.md Outdated Show resolved Hide resolved
0000-template.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

A few things that come to mind:

  • This is a bit too external-facing. I feel like we need to integrate the internal RFC process into this.
  • How do we allocate a shepherd for RFCs? Is that based on the ownership areas? What about stuff that doesn't fall under a specific ownership? Is this part of the support workflow?
  • How does this integrate with our public roadmap? What's the prioritization process for RFCs we want to work on as a core team? Eventually I think these RFCs are going to be the main feed of our quarterly planning, so we need to make sure that we have a backlog of "ready to implement" RFCs (is that "active"?) so we can pick ones for squad implementation every quarter.
  • Public/private - I have no doubt that we will eventually have some RFCs that we don't feel comfortable exposing publicly (e.g. internal support for CDK etc). Where do we handle/manage them?

0000-template.md Outdated

# Motivation

Why are we doing this? What use cases does it support? What is the expected
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want these to be <!-- comments --> so it's clear they are just instructions?

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the problem with just replacing the text?

Copy link
Contributor

Choose a reason for hiding this comment

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

I find it easier to work on the doc in pieces and it wrapping the guidelines with <!-- comments --> make it simpler to tell which sections I've already filled in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only thing that I worry about is not being able to easily read the template when rendered in the browser. My guess is that people will be reviewing the contents of the template on github before cloning the repo and working on it. I'm still open to this if you think its better with that in mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, so how about using a block quote (>) for the guidelines?

Like this

0000-template.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
The "RFC" (request for comments) process is intended to provide a
consistent and controlled path for new features to enter the project.

[Active RFC List](https://github.com/awslabs/aws-cdk-rfcs/pulls)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit confused by the terminology of "active". doesn't "active" mean that it was already merged?

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 active means "under discussion" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"active" in rust/react's RFC process means, the RFC PR has been merged and it is ready for someone to work on implementation.

It looks like they also use the term "active" to refer to RFCs that are being discussed and haven't been merged. My vote is we change that to "Pending". So this would say "Pending RFC List".

Alternatively, we can change RFCs that have been merged and are ready to implement to be called "Resolved". That may connote too much finality though and may falsely indicate that the feature has been implemented already. Either way we should come up with nice simple names for the states for RFCs.

How about:

  1. Pending - RFC PR is created and under review
  2. Final Comment Period - RFC has been reviewed and is open for final comments before acceptance.
  3. Active - RFC PR has been accepted and merged, ready for someone to implement.
  4. Resolved - Implementation is done.

We may or may not want to have the "resolved" state/label. Are we going to come back to the RFC repo and resolve RFCs once implementations are finished?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say we should split 1 to:

  1. Pending - waiting for a review
  2. Under review - currently being reviewed

To reflect the team/community engagement in the RFC repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am good with what you proposed, albeit those are not 100% intuitive to me (particularly “active” which has multiple intuitive connotations), but I see the value in using terminology close to existing projects.

I’d also like to formalize the state of projects that are still in the “idea” stage. Those will only have an issue with motivation in them but no RFC yet. For example, most of the issues we have in the repo right now are at this stage, and I think it will be super powerful to make them discoverable.

The beginning of the README should have a list of query links that will be easy to find all tracking issues for RFCs in each state.

README.md Outdated
Comment on lines 21 to 24
- Any change to existing APIs that would break existing code.
- The removal of existing features or public APIs.
- The introduction of new idiomatic usage or conventions, even if they
do not include code changes to React itself.
Copy link
Contributor

Choose a reason for hiding this comment

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

Other examples:

  • Changes to core team workflows
  • Features that cross the construct-library
  • Framework capabilities
  • Formal specifications (like the cloud assembly, tree.json, etc).

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
0000-template.md Outdated

# Motivation

Why are we doing this? What use cases does it support? What is the expected
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the problem with just replacing the text?

0000-template.md Outdated
choosing them?
- What is the impact of not doing this?

# Prior Art
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel prior art should go before the proposed solution.

0000-template.md Outdated Show resolved Hide resolved
README.md Outdated
The "RFC" (request for comments) process is intended to provide a
consistent and controlled path for new features to enter the project.

[Active RFC List](https://github.com/awslabs/aws-cdk-rfcs/pulls)
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 active means "under discussion" ?

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
@MrArnoldPalmer MrArnoldPalmer force-pushed the feature/define-process branch 2 times, most recently from 27cdf4b to 8dd8f2a Compare December 18, 2019 21:48
@MrArnoldPalmer MrArnoldPalmer force-pushed the feature/define-process branch 8 times, most recently from 3859873 to 96f6db3 Compare December 30, 2019 19:33
Co-Authored-By: Romain Marcadier-Muller <[email protected]>
Adds code of conduct and notice files.
@MrArnoldPalmer MrArnoldPalmer force-pushed the feature/define-process branch from 1663221 to ac4606e Compare January 2, 2020 00:42
Copy link
Contributor

@NetaNir NetaNir left a comment

Choose a reason for hiding this comment

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

If I'm understanding correctly we will have a separate repo for RFCs that will host RFC's in all stages: pending , under review, final comments and active. One desired outcome of such repo will be that active RFCs will be picked up by the community. In order to appeal to wider audience I suggest that the repo README will have a short intro which explains RFC's, how are they created and how will the process of implementing an RFC looks like - emphasizing that a CDK team member will shepherd the process and will supply guidance and technical mentorship. The purpose of such intro is to avoid making the RFC implementation process intimidating and to encourage the community to participate. While I like the use of the formal RFC terminology I think we could benefit from loosening it a bit.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated
The "RFC" (request for comments) process is intended to provide a
consistent and controlled path for new features to enter the project.

[Active RFC List](https://github.com/awslabs/aws-cdk-rfcs/pulls)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would say we should split 1 to:

  1. Pending - waiting for a review
  2. Under review - currently being reviewed

To reflect the team/community engagement in the RFC repo.

README.md Outdated
be prepared to make revisions in response.
- Build consensus and integrate feedback. RFCs that have broad support are
much more likely to make progress than those that don't receive any comments.
- Eventually, the team will decide whether the RFC is a candidate for
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a reference to an existing 'good' RFC could be helpful

@eladb
Copy link
Contributor

eladb commented Jan 2, 2020

I think @NetaNir has an interesting point. In addition to managing the RFC process, I agree that we want this repo and process to be a place where community can discover ideas for CDK and jsii-related projects to contribute. It's related to my comment about an additional "idea" stage and a quick link to a query that people can use to find ideas, and I really like @NetaNir's idea to add "T shirt size estimation" to each RFC, so people can discover ideas in different levels of complexity.

Also, there's a point about the "RFC" terminology that might not be familiar to many people (in the CDK community), and so I agree that it makes sense to make it a bit more accessible and have less of a bureaucratic feel to it by lightning the language a little.

@MrArnoldPalmer
Copy link
Contributor Author

I'm onboard with an intro section and lightening it up. As far as sizing goes we can create some labels and the core team shepherd can provide a rough estimate after implementation issues are created. We can also create a "good first project" label or something along those lines.

@MrArnoldPalmer MrArnoldPalmer requested a review from NetaNir January 3, 2020 00:52
Shorten the intro section and explain the repos use for tracking the
state of upcoming features. Add "What does all this mean", "What is an
RFC", and "Contributions" section to further encourage community
participation.
@MrArnoldPalmer MrArnoldPalmer force-pushed the feature/define-process branch from e0887e0 to 814fcfc Compare January 3, 2020 01:16
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

Nit: I think the title of this RFC should be "RFC Process" and not "Define RFC Process", no?

.github/ISSUE_TEMPLATE/tracking-issue.md Outdated Show resolved Hide resolved
0000-template.md Outdated

# Motivation

Why are we doing this? What use cases does it support? What is the expected
Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, so how about using a block quote (>) for the guidelines?

Like this

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@MrArnoldPalmer MrArnoldPalmer changed the title RFC: #16 Define RFC Process RFC: #16 RFC Process Jan 3, 2020
@MrArnoldPalmer MrArnoldPalmer requested a review from eladb January 3, 2020 23:41
Add quicklinks to RFC tracking issues in various states. Cleanup some
formatting. Move bodies of template sections into blockquotes.
@MrArnoldPalmer MrArnoldPalmer force-pushed the feature/define-process branch from 01c7ac7 to 4470b03 Compare January 4, 2020 00:16
eladb
eladb previously approved these changes Jan 5, 2020
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

YASS!

README.md Outdated Show resolved Hide resolved
@MrArnoldPalmer MrArnoldPalmer added the status/final-comment-period Pending final approval label Jan 5, 2020
Co-Authored-By: Elad Ben-Israel <[email protected]>
@MrArnoldPalmer MrArnoldPalmer dismissed stale reviews from eladb via 7119d3d January 5, 2020 19:06
@MrArnoldPalmer MrArnoldPalmer merged commit 1c683b4 into master Jan 8, 2020
@MrArnoldPalmer MrArnoldPalmer deleted the feature/define-process branch January 8, 2020 16:47
@MrArnoldPalmer MrArnoldPalmer added status/done Implementation complete and removed status/final-comment-period Pending final approval labels Jan 8, 2020
@eladb
Copy link
Contributor

eladb commented Jan 8, 2020 via email

@eladb eladb changed the title RFC: #16 RFC Process RFC of RFCs Jan 27, 2020
@eladb eladb changed the title RFC of RFCs RFC: #16 RFC Process Jan 27, 2020
@eladb eladb changed the title RFC: #16 RFC Process RFC: 0016 RFC Process Jan 27, 2020
@eladb eladb removed the status/done Implementation complete label Jan 27, 2020
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.

5 participants