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 RFC template #4236

Merged
merged 13 commits into from
Dec 20, 2019
Merged

Conversation

ericclemmons
Copy link
Contributor

As I was working on #4235, I would've benefited from a template based on prior RFCs:

(There are others, but they didn't follow a common format)


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

@ericclemmons ericclemmons added feature-request Request a new feature Tooling Related to Tooling labels Oct 22, 2019
@codecov
Copy link

codecov bot commented Oct 22, 2019

Codecov Report

Merging #4236 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4236   +/-   ##
=======================================
  Coverage   76.24%   76.24%           
=======================================
  Files         174      174           
  Lines        9619     9619           
  Branches     1964     1906   -58     
=======================================
  Hits         7334     7334           
- Misses       2140     2148    +8     
+ Partials      145      137    -8
Impacted Files Coverage Δ
packages/core/src/OAuthHelper/GoogleOAuth.ts 31.25% <0%> (ø) ⬆️
...ackages/pubsub/src/Providers/AWSAppSyncProvider.ts 66.66% <0%> (ø) ⬆️
...pubsub/src/Providers/AWSAppSyncRealTimeProvider.ts 18.64% <0%> (ø) ⬆️
...rc/Providers/AmazonAIConvertPredictionsProvider.ts 61.23% <0%> (ø) ⬆️
packages/xr/src/Providers/SumerianProvider.ts 47.55% <0%> (ø) ⬆️
packages/core/src/OAuthHelper/FacebookOAuth.ts 34.09% <0%> (ø) ⬆️
packages/auth/src/OAuth/OAuth.ts 48.48% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa8a01e...f663839. Read the comment docs.

Copy link
Contributor Author

@ericclemmons ericclemmons left a comment

Choose a reason for hiding this comment

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

The current format is based on existing RFCs, but there are some beneficial components that we can borrow from https://github.com/reactjs/rfcs/blob/master/0000-template.md.

  1. Drawbacks – Helpful for determining what is outside of scope or maintainability, as well as downstream impacts.

  2. Detailed Design – Should be enough for someone to implement based off of, including corner-cases & exceptions where the proposal wouldn't apply.

  3. Alternatives – What can meet the needs today, or in the future if the proposal is not accepted.

@ashika01
Copy link
Contributor

Love this @ericclemmons. 🎉
Just couple of thoughts:

  • I think in the Proposed Solution bit, we should have a high level architecture and the detailed design and its sample implementation or at least enough context / sample for anyone with familiarity to implement this.
  • Also we need to add a section to discuss pros and cons for the solution, compare and contrast the existing solution and solution's comparison with industry practices (not sure how to name this section).

@ericclemmons
Copy link
Contributor Author

You're spot-on, @ashika01. I'll incorporate your feedback.

Whether it's an RFC or task, I'm a big fan of laying out the game-plan for anyone to execute. It's more effort upfront, but resolving that ambiguity early can make the difference in even needing the feature or not 🤞

@ericclemmons ericclemmons self-assigned this Nov 6, 2019
Copy link
Contributor

@ashika01 ashika01 left a comment

Choose a reason for hiding this comment

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

Looks great. Just left a suggestion. But I think this is a great start. 🎉 🏅

Copy link
Contributor

@Amplifiyer Amplifiyer left a comment

Choose a reason for hiding this comment

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

This is great! Ship it!

.github/ISSUE_TEMPLATE/rfc.md Outdated Show resolved Hide resolved
Copy link
Contributor

@sammartinez sammartinez left a comment

Choose a reason for hiding this comment

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

Overall, this is great! One callout that is critical for this PR. I just want to see about adding a note that makes it clear if any of the pieces of the structure is missing, we will close the RFC and ask the customer to open a feature request

.github/ISSUE_TEMPLATE/rfc.md Outdated Show resolved Hide resolved
@sammartinez
Copy link
Contributor

sammartinez commented Nov 12, 2019

Overall, this is great! One callout that is critical for this PR. I just want to see about adding a note that makes it clear if any of the pieces of the structure is missing, we will close the RFC and ask the customer to open a feature request

LGTM. Just want to make sure we have the note about, "If we do not have all sections filled out, this RFC will be closed in favor of a feature request" Or something to that nature.

@ericclemmons
Copy link
Contributor Author

ericclemmons commented Nov 14, 2019

The wording could come across a bit harsh. I think the best thing could be to clarify the difference between this & https://github.com/ericclemmons/amplify-js/blob/github-rfc-template/.github/ISSUE_TEMPLATE/feature_request.md, so that users can make the correct choice.

Honestly, the RFC template seems like the feature request template on steroids, but what I appreciate about the feature request is that it has a low bar for "I wish Amplify did X because of Y". (RFCs generally have a higher bar for larger, impactful changes, and is closer to what we would write for internal review).

What are your thoughts on solving the distinction issue on https://github.com/aws-amplify/amplify-js/issues/new/choose?

@ashika01
Copy link
Contributor

@ericclemmons: @sammartinez and I talked briefly about this. I think we should merge ur template for now and encourage people to submit RFC and mentor to write better RFC. We will have a WIKI page on life cycle of RFC and Acceptance Criteria.

@sammartinez
Copy link
Contributor

The wording could come across a bit harsh. I think the best thing could be to clarify the difference between this & https://github.com/ericclemmons/amplify-js/blob/github-rfc-template/.github/ISSUE_TEMPLATE/feature_request.md, so that users can make the correct choice.

Honestly, the RFC template seems like the feature request template on steroids, but what I appreciate about the feature request is that it has a low bar for "I wish Amplify did X because of Y". (RFCs generally have a higher bar for larger, impactful changes, and is closer to what we would write for internal review).

What are your thoughts on solving the distinction issue on https://github.com/aws-amplify/amplify-js/issues/new/choose?

Understood and part of it is customers may end up just removing the RFC template in general for a feature request tag. Im fine to get this in.

Copy link
Contributor

@sammartinez sammartinez left a comment

Choose a reason for hiding this comment

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

LGTM 🌮

@ericclemmons
Copy link
Contributor Author

Thanks for the input everyone! Let's see how this works out in the wild and adjust from there.

In the meantime, this should help us have thorough RFCs in the open for customers to help provide feedback on. 🙏

@ericclemmons ericclemmons merged commit eaff2d1 into aws-amplify:master Dec 20, 2019
@ericclemmons ericclemmons deleted the github-rfc-template branch December 20, 2019 19:47
@github-actions
Copy link

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request a new feature Tooling Related to Tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants