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

Bug 1879663 - Add RFC proposing updates to the RFC process #5681

Merged
merged 2 commits into from Mar 13, 2024

Conversation

MatthewTighe
Copy link
Contributor

@MatthewTighe MatthewTighe commented Feb 21, 2024

This patch introduces a number of small updates to the RFC process. I've selected a long initial deadline since this is just a process change and isn't blocking any work, plus things have been chaotic the last couple weeks.

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Breaking Changes: If this is a breaking Android Components change, please push a draft PR on Reference Browser to address the breaking issues.

To download an APK when reviewing a PR (after all CI tasks finished running):

  1. Click on Checks at the top of the PR page.
  2. Click on the firefoxci-taskcluster group on the left to expand all tasks.
  3. Click on the build-apk-{fenix,focus,klar}-debug task you're interested in.
  4. Click on View task in Taskcluster in the new DETAILS section.
  5. The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

https://bugzilla.mozilla.org/show_bug.cgi?id=1879663

@github-actions github-actions bot added the 🕵️‍♀️ needs review PRs that need to be reviewed label Feb 21, 2024
Copy link
Collaborator

@jonalmeida jonalmeida left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! Left some feedback below that revolves around focusing on the new additions. I'm slightly concerned that we making this more formal than it needs to be and having the inverse effect you are trying to avoid (i.e. slowing down development).

Let's also focus on making this change more about the new additions and not what already exists.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the difference between this template and the previous one? The prior one worked as a way to introduce the process and be used as a template.

Can we just amend that one with additional information that might be missing?

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 value add here would just be around clarity: the new template has detail about what each section is for, the file is clearly named, and has additional sections that have been pulled from other examples. How would we amend the original with additional templating information without diluting the proposal within it?

I also think there is value in treating an accepted RFC as finalized and not updating it in place after the fact, as I've come to learn 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since it is meant as a template, like our previous case, we can still amend it with annotations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it is meant as a template, like our previous case, we can still amend it with annotations.

Is there an example of somewhere we've done that already? My hope is that the template and README become living, update-able documents that are clear entry points to the process. I think if we start amending RFCs, then it becomes less clear which are the most up-to-date, and begs the question of whether they serve as living documentation for the systems and changes they had proposed.


- The scale of the Firefox Android team has grown substantially since the RFC process was first introduced.
- More external teams are using and contributing to Android Components.
- Approval or rejection of a proposal has required periods of time that can slow implementation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The approval or rejection time is set by the author already. The RFC is just a pull request that goes through the same process as a code review: if you have an approval, you can merge that work. Additional reviewers are welcome to add feedback that is up to the author to consider if they want to address it in the current PR, future work, or not applicable.

I'm not sure this is a valid motivation. What is the difference that we are adding here?

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 intent here is clarity and centralization of information. Setting a deadline in a different communication channel divorces that information from the proposal if the proposal is shared directly, for example.

My goal is to make these easier to write for people unfamiliar with the process, and codifying things like this I think will assist in knowledge transfer - future authors will have less reliance on offline (and usually hidden from public/history) mentorship.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you mean, "Non-explicit timeline for proposal deadlines."

Approval or rejection to slowdown the process is the intention of an RFC.

- The scale of the Firefox Android team has grown substantially since the RFC process was first introduced.
- More external teams are using and contributing to Android Components.
- Approval or rejection of a proposal has required periods of time that can slow implementation.
- Stakeholders have either been implicit by sharing RFCs in various communication channels, or an author must be expected to correctly select reviewers that lack explicitly documented ownership of affected areas.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's fine to have secondary channels of communication for feedback - everyone has their own way. The missing step that I've seen in practice, is that developers forget to come back to the origin (their original doc or bug or PR), and document the communication and it's outcome.

If we want specific feedback from a person, we add them as a reviewer to the PR today. It's up to the author to decide then if they want to wait for that feedback or they are happy with the review of another reviewers.

Below it says, "explicit stakeholders for RFCs" which contradicts, "an author must be expected to correctly select reviewers.."

I'd recommend instead making this into something similar to: "New RFCs may not be visible to the entire team to have equal time to consider."

The proposal could then be: "Share the RFC to the mailing list/slack channel with the deadline date."

For prior art of what I'm suggesting, see the web standards process or the services-rfcs@ mailing list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's fine to have secondary channels of communication for feedback - everyone has their own way.

I am hoping this removes some of the guesswork in finding out what every reviewer's or author's individual way is, especially for people that are new to the process or working across teams/areas of ownership.

Below it says, "explicit stakeholders for RFCs" which contradicts, "an author must be expected to correctly select reviewers.."

In the way that selecting explicit stakeholders and reviewers is basically the same thing? I do agree here, but I am hoping with a revised CODEOWNERS file that this is improved and that by offering the suggestion as part of the guide and template, it will give authors an explicit step in seeking feedback.

The proposal could then be: "Share the RFC to the mailing list/slack channel with the deadline date."
For prior art of what I'm suggesting, see the web standards process or the services-rfcs@ mailing list.

The services-rfcs mailing list looks great. I would definitely like to include something like that as part of the guide for distribution of these, but I don't feel like the firefox-android mailing list is all-inclusive enough especially as we talk about other teams potentially taking ownership of specific areas. Do you have any suggestions, or should we make a new one?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am hoping this removes some of the guesswork in finding out what every reviewer's or author's individual way is, especially for people that are new to the process or working across teams/areas of ownership.

My impression is that we are solving for the wrong problem then. 🤷

In the way that selecting explicit stakeholders and reviewers is basically the same thing?

The "motivation" section is a list of problems to solve, so by saying the problem to solve here is that a reviewer is expected to pick a reviewer, and then the "Guide-level explanation" (recommended solution) is to "[have/select/pick a] explicit stakeholders for RFCs". These are contradicting statements.

Do you have any suggestions, or should we make a new one?

We should avoid making more channels for people to subscribe to: let's just use firefox-mobile@ and mobile-android-team@.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My impression is that we are solving for the wrong problem then. 🤷

Is there another problem you think we should be solving for? I am basing this proposal off my own and other's feedback that a lack of clarity has lowered engagement and speed of this process, but I am open to try and solve additional problems with this.

docs/rfcs/0013-rfc-process-updates.md Outdated Show resolved Hide resolved
docs/rfcs/0013-rfc-process-updates.md Outdated Show resolved Hide resolved
docs/rfcs/README.md Show resolved Hide resolved
Copy link
Contributor Author

@MatthewTighe MatthewTighe left a comment

Choose a reason for hiding this comment

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

Thanks for the reviews!

I'm slightly concerned that we making this more formal than it needs to be and having the inverse effect you are trying to avoid (i.e. slowing down development).

I think this is a fair concern. My hope is that introducing a little more formality will make the process more accessible to people unfamiliar with it (or with the people who might engage in it). Longer-term Mozillians could probably just write proposals for each other all day because we better understand each other's work styles and expectations, but I'm hoping writing those down a bit will actually remove barriers to entry.

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 value add here would just be around clarity: the new template has detail about what each section is for, the file is clearly named, and has additional sections that have been pulled from other examples. How would we amend the original with additional templating information without diluting the proposal within it?

I also think there is value in treating an accepted RFC as finalized and not updating it in place after the fact, as I've come to learn 😅


- The scale of the Firefox Android team has grown substantially since the RFC process was first introduced.
- More external teams are using and contributing to Android Components.
- Approval or rejection of a proposal has required periods of time that can slow implementation.
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 intent here is clarity and centralization of information. Setting a deadline in a different communication channel divorces that information from the proposal if the proposal is shared directly, for example.

My goal is to make these easier to write for people unfamiliar with the process, and codifying things like this I think will assist in knowledge transfer - future authors will have less reliance on offline (and usually hidden from public/history) mentorship.

- The scale of the Firefox Android team has grown substantially since the RFC process was first introduced.
- More external teams are using and contributing to Android Components.
- Approval or rejection of a proposal has required periods of time that can slow implementation.
- Stakeholders have either been implicit by sharing RFCs in various communication channels, or an author must be expected to correctly select reviewers that lack explicitly documented ownership of affected areas.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's fine to have secondary channels of communication for feedback - everyone has their own way.

I am hoping this removes some of the guesswork in finding out what every reviewer's or author's individual way is, especially for people that are new to the process or working across teams/areas of ownership.

Below it says, "explicit stakeholders for RFCs" which contradicts, "an author must be expected to correctly select reviewers.."

In the way that selecting explicit stakeholders and reviewers is basically the same thing? I do agree here, but I am hoping with a revised CODEOWNERS file that this is improved and that by offering the suggestion as part of the guide and template, it will give authors an explicit step in seeking feedback.

The proposal could then be: "Share the RFC to the mailing list/slack channel with the deadline date."
For prior art of what I'm suggesting, see the web standards process or the services-rfcs@ mailing list.

The services-rfcs mailing list looks great. I would definitely like to include something like that as part of the guide for distribution of these, but I don't feel like the firefox-android mailing list is all-inclusive enough especially as we talk about other teams potentially taking ownership of specific areas. Do you have any suggestions, or should we make a new one?

docs/rfcs/README.md Show resolved Hide resolved
docs/rfcs/0013-rfc-process-updates.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@jonalmeida jonalmeida left a comment

Choose a reason for hiding this comment

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

I am still not convinced in the value of the duplication and some of the wording, but not enough to block on this work if it's intentional.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since it is meant as a template, like our previous case, we can still amend it with annotations.


- The scale of the Firefox Android team has grown substantially since the RFC process was first introduced.
- More external teams are using and contributing to Android Components.
- Approval or rejection of a proposal has required periods of time that can slow implementation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you mean, "Non-explicit timeline for proposal deadlines."

Approval or rejection to slowdown the process is the intention of an RFC.

- The scale of the Firefox Android team has grown substantially since the RFC process was first introduced.
- More external teams are using and contributing to Android Components.
- Approval or rejection of a proposal has required periods of time that can slow implementation.
- Stakeholders have either been implicit by sharing RFCs in various communication channels, or an author must be expected to correctly select reviewers that lack explicitly documented ownership of affected areas.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am hoping this removes some of the guesswork in finding out what every reviewer's or author's individual way is, especially for people that are new to the process or working across teams/areas of ownership.

My impression is that we are solving for the wrong problem then. 🤷

In the way that selecting explicit stakeholders and reviewers is basically the same thing?

The "motivation" section is a list of problems to solve, so by saying the problem to solve here is that a reviewer is expected to pick a reviewer, and then the "Guide-level explanation" (recommended solution) is to "[have/select/pick a] explicit stakeholders for RFCs". These are contradicting statements.

Do you have any suggestions, or should we make a new one?

We should avoid making more channels for people to subscribe to: let's just use firefox-mobile@ and mobile-android-team@.

docs/rfcs/0013-rfc-process-updates.md Outdated Show resolved Hide resolved
@gabrielluong gabrielluong added approved PR that has been approved and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Feb 27, 2024
docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
* RFC PR: [<PR #>](https://github.com/mozilla-mobile/android-components/pull/<#>)
* Start date: YYYY-MM-DD (Day of proposal posting.)
* End date: YYYY-MM-DD (Initial deadline for feedback, subject to blocking changes required. The proposal can be merged immediately after stakeholder approval.)
* Stakeholders: <github-username>, <github-username>
Copy link
Contributor

@t-p-white t-p-white Feb 28, 2024

Choose a reason for hiding this comment

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

<github-username> doesn't show up in the fie view, is that expected?
Screenshot 2024-02-28 at 10 21 48
Oh, it also isn't displayed in comments either without the quotes but then I guess with quotes the desired functionality won't work 😞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, thanks

Copy link
Contributor

@t-p-white t-p-white 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 few 'optional' sections, perhaps we could hightlight that in the section title instead of in the section description? e.g. something like:

  • Drawbacks (Optional)
  • *Drawbacks

docs/rfcs/README.md Outdated Show resolved Hide resolved

An initial deadline should be included in each RFC. This should usually be at least a week, so plan accordingly.
For more substantial changes, it can be useful to push that to 2 or 3 weeks so that there is more opportunity for feedback from people that are not stakeholders.
Blocking feedback can push these deadlines back, but they should be updated appropriately so that interested parties have a clear understanding of timelines.
Copy link
Contributor

@t-p-white t-p-white Feb 28, 2024

Choose a reason for hiding this comment

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

Based on my comment above, I'm not sure if we should say pushing deadlines back, it's more that the deadline for general feedback is gone & this is now dealing with any blockers. Expectations can still be set with stakeholders but calling this a 'deadline' becomes a moving target which I don't think adds any value here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks this is useful feedback! Updated accordingly

Copy link
Contributor

@t-p-white t-p-white left a comment

Choose a reason for hiding this comment

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

Nice job @MatthewTighe, great that you took the initiative to put this together, should hopefully make RFCs a little more user-friendly going forward 🙂 just a few general comments from me


## How to contribute an RFC

There is a [template](./0000-template.md) that can be useful for structure. While drafting
Copy link
Contributor

@t-p-white t-p-white Feb 28, 2024

Choose a reason for hiding this comment

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

While drafting a proposal, consider the scope of your changes. Generally, the higher the impact the changes will have on downstream consumers of APIs, other teams, or users the more detail the RFC should include. Detail would be things like example use cases, prototypes, and considered alternatives.

In an effort to keep this README concise, I wonder if this information is adding anything the new template isn't? Or perhaps this should move to the template?

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 think this is useful general advice, and that it would probably need to be repeated in a few sections in the template so I've kept it here for now but tried to streamline it a bit. LMK if you feel strongly it should change

Copy link
Collaborator

@jonalmeida jonalmeida left a comment

Choose a reason for hiding this comment

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

Circling back to review the latest changes made and one I missed (sorry!).

- The scale of the Firefox Android team has grown substantially since the RFC process was first introduced.
- More external teams are using and contributing to Android Components.
- Implicit deadlines have encouraged a lack of engagement with RFCs, slowing follow-up work.
- There has been a low level of engagement with RFCs because of a lack of clarity around the process and when they are appropriate.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- There has been a low level of engagement with RFCs because of a lack of clarity around the process and when they are appropriate.
- There has been a low level of engagement with RFCs because of a lack of clarity around the process.

When they are appropriate is on the first RFC, so I don't think this makes sense to add. The template you are introducing does not solve the "when" problem either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I had hoped the README (as a slightly more abstract document which would serve as a landing place in the directory) would answer that question. Should the two documents just be combined?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure. I have no strong preferences, so feel free to add it as a follow-up.

Just make sure we align it with the 001 RFC to avoid confusion since we are two places now that offer recommendations.

docs/rfcs/0000-template.md Outdated Show resolved Hide resolved

This section should include any drawbacks to the proposal.

## Rationale and alternatives (optional)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
## Rationale and alternatives (optional)
## Rationale and alternatives

Sorry, I missed this in my previous pass. For the same reason as the comment above on 'Unresolved questions', let's make it required that the developer explicitly says there are no alternatives to their design.

@MatthewTighe MatthewTighe force-pushed the rfc-improvements branch 2 times, most recently from 889198d to 4450e7b Compare March 1, 2024 23:21
@MatthewTighe MatthewTighe added the 🛬 needs landing PRs that are ready to land label Mar 13, 2024
@MatthewTighe
Copy link
Contributor Author

Landing this before the deadline since the monorepo merge is scheduled this week and the stakeholders have approved

@mergify mergify bot merged commit 024c307 into mozilla-mobile:main Mar 13, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved PR that has been approved 🛬 needs landing PRs that are ready to land
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants