Could/Should we please "protect" the quick docs repo so that all changes require PRs?

Hi folks,

We’re seeing more folks contribute to quick-docs, which is awesome. When someone has a PR merged, i also add them to the quick-docs-committers group. This is both a reward for their contribution, and it’s also a good way of increasing the team—everyone in this group should get notifications of issues/prs so they can help out where they want to. Folks that are experts in certain topics, like printing etc. can also quickly make required updates without always having to wait for review.

However, all of us in the quick-docs-committers group can directly commit to the main branch. This isn’t really a problem yet, but as more folks start to contribute, it can sometimes lead to issues/conflicts because of lack of communication/awareness because we don’t get notifications for commit pushes. This means that the team won’t be aware of a change until they actually read a page. It can (very unlikely) also be a security issue—if a malicious operator gains access to one of the committers keys/accounts/systems, they can directly push things to the main branch without the knowledge of the team.

In short, ideally, the whole team should be aware of all pushes to the main branch.

So, can we maybe stop any direct commits to main and enforce a workflow that requires all changes should be made via PRs? Creating a new branch and opening a PR isn’t really that much work, and even if folks go ahead and merge trivial PRs quickly, at least it ensures that everyone is aware of the change and that there’s always a window for other team members to review them?

On pagure, one can go to “project options” in settings and enable “Pull-request access only”, which, according to the documentation that comes up if you click on it right at the bottom of the page says: “Pagure supports blocking direct commit access to a project and enforcing all changes to a project to be done via pull-requests.”

This requires someone from the docs team on pagure to update the settings. (I’m in the committers group which has commit but not admin access)

+1 from me for “pull-request access only”. In addition to protective aspect, PR based process encourages editorial review and collaboration that should work for most types of docs contributions.

@pbokoc @pboy what do you think?

1 Like

We had this discussion about a year ago, but unfortunately didn’t came to a final decision and solution.

Unfortunately, we are on discussion, and so I was stripped of my well-structured and categorized mail archive for Docs. But if I remember correctly:

  • we agreed that a PR workflow is the default way of handling updates and additions
  • According to analyses at the time, there were a number of authors who systematically and continuously dealt with certain articles and topics. We did not want to disrupt their workflow, which included the possibility of a direct commit.
  • To prevent the group from getting out of hand, one idea was to remove authors from the Committers group if they have not contributed over a certain period of time, e.g. the last 3 or 5 years. This would “slim down” the group in the long term and concentrate it on active contributors.
  • We agreed that we need a strong group of active committers to reduce the long delays in PRs.
  • And we agreed that committers should also use a PR workflow in “normal cases” and that a direct commit should be limited to special cases. And committers themselves should be motivated enough to adhere to this workflow.

Maybe someone has the patience to search for the old discussions and post the links here.

Perhaps it’s time to put these ideas into practice and restructure the Committers group accordingly.

  • A first step could be to search the current group for inactive authors.
  • A second step could be to set specific expectations for membership, e.g. at least 3 commits/reviews per year with the exception for someone who has been working on a specific topic for several years and only contributes to it.
  • I really appreciate the idea of rewarding. But perhaps we should first assign a badge for each commit and then, for example, accept them into the group when they have 3 commit-badges. At the moment that may be an overkill, but in the long term that could be beneficial.
  • And finally, we should start monitoring the PRs and direct commits. We could pursue the goal of analyzing the development of the previous 6 months at the end of February, for example. We could then use empirical data to examine the actual benefits of preventing direct commits.

Sorry, the text is a bit long.

Requiring a Pull Request workflow makes sense to me. There should also be an easy way for a submitter to find out who can review their PR and where to go if someone does not automatically jump on it.

I am aware that the “active vs. inactive” members of a group is a conversation that often happens in Fedora teams, and it can take a lot of time and energy that could be spent leveling up the folks who are already active. I would be cautious about trying to perfect a process before we have a lot of momentum behind it.

1 Like

Yeah, agreed to that strategie. But what are the consequences for the next steps?

We could triage PRs in discourse using relevant tags for visibility. Tagging in Quick Docs repo doesn’t get timely response.

Does this happen because it is not clear who is supposed to review Quick Docs PRs? Any committer can review, but because it can be any committer, rarely does someone feel empowered to review and merge. This seems less like a tooling challenge and more like we have not empowered someone to lead.

Instead of coming up with a new workflow, what if we instead took a new approach with the existing workflow? What does someone or a group of people need to feel empowered to “own” the Quick Docs repo PRs? :muscle:

One idea would be to come up with a monthly “wrangler” or triage team , who keeps an eye on Quick Docs PRs for a month. That wrangler or triage team either reviews and merges when they can verify changes, or they pull in a subject-matter expert to review if they are unable to verify changes. At the end of the month, the wrangler or triage team would write short “handover” notes :memo: for someone else who could then take over where the previous wrangler left off.

I am thinking aloud. These ideas are not fully-developed yet. :thought_balloon:

We need a facilitator(s) to triage PRs to right team/subject matter expert.
Tagging in discourse instead of dev mailing list is a way to help triage PRs.

Previously, dev mailing list was surfaced a few months ago.

That chimes in well with my line of thought. We could group categories into a pool of reviewers (all on a voluntary basis). For example,

  • SELinux
  • Kernel
  • FIDO
  • Kubernetes
  • Pipewire

My concern is that using two platforms to coordinate one change introduces complexity. Instead of opening a Pull Request and keeping the conversation on Pagure, we are splitting the conversation between two platforms. We ask people to follow an extra step of creating a review thread on Fedora Discussion in addition to the Pagure PR. I feel like this is a lot to ask for, especially with a new contributor. We want the process to be as lightweight as possible and to avoid running the new contributor through too many hoops.

Could we have the facilitator(s) triage PRs in Pagure and escalate to a Fedora Discussion topic if something gets stuck or blocked?

Again, my concern is not that Pagure is causing PRs to go unanswered, but instead that we have not empowered people to feel like they can be that facilitator role. Before reinventing the process, I want to look first on how we empower someone to facilitate and confidently review changes. I am not sure we have solved this part of the equation yet.

I think I am missing context. I am not sure what you are referring to. Could you elaborate?

I like this idea a lot. Finding subject-matter experts means we can identify names of specific people who can review specific content. These people do not have to even be a member of the Docs committer group if they prefer not to, but we can loop them in as a subject-matter expert whenever there are content changes that require input from an expert.

Having a list of people who are also not everyday committers increases the value of being a maintainer. Someone can volunteer to be an editor for SELinux or Pipewire without signing up for every single PR and issue on the Quick Docs team. The role of the facilitator would be to connect the subject-matter expert to a proposed change when it is applicable.

Tagging in discourse is not intended for a new contributor. Agreed not to split the conversation.

Dev mailing list was an idea cropped up by you and Ankur/Peter on how to get help from subject matter specialists in other working groups.

I’d say empowerment in most contributions would come from a sense of achievement and potential reward (skill development and other personal benefits). If there is a good match, a contributor would re-prioritize/re-focus the type of contributions.

Look at patterns of unanswered questions on Fedora Ask. I doubt if there is a lack of expertise. Could be visibility issues and not highlighting risks for those going unanswered.

If we step back on the main point of this thread, we need to act on PR workflow by default and how to reduce bottleneck (PRs pilng up). We lack reviewers.

I don’t think using PRs does. They can open the PR, and then merge it. It only takes another extra minute. The advantage is that the whole team then also gets notified that these people have made changes, which I think is quite valuable.

Like Justin said, this comes up a lot (and will continue to come up), but the energy used in cleaning up could be better spent helping the active folks. I’m not really worried about this either. It doesn’t matter if there are lots of inactive folks in teams as long as there are enough active folks to keep it going. If all changes are visible, we’ll always be able to see who did what.

It’s a good idea, but how does this work? If we already had reviewers in these areas, we could go ahead and group them, but since we don’t, how do we come up with these groups?

For the time being, I e-mail the -devel list for help. Sometimes it works, sometimes it doesn’t.

While we discuss this, could we please enable the PR only workflow in the quick-docs repo?

“increasing reviewers” is not something that can be done in a short period of time, but I don’t think that needs to block other simple tasks like this one.

in addition to the -devel mailing list, we could refer to metadata on Quick Docs (authors), reviewers on PR comment, and consult with QA team or SIGs (tag individuals if reviewers know subject matter expert).

I’d say the most up to date information can be found at the Flock speakers list and top helpers in discourse (you just need to search by keywords). I would DM to speakers on the subject matter if they can comment on particular PRs or recommend other persons.

I don’t have permissions to make changes to the repo settings. Could @pboy or @pbokoc
enable it?

Although we don’t have definitive list for subject matter expert in all Quick Docs PRs and Fedora Ask categories, if we start finding and asking right people (I know I have to ask around) and tagging them on appropriate PRs, we will find it easier to ask for help next time.

So, the idea of adding folks to the quick-docs-committers group is that they all get notifications from issues/PRs. So, if the folks that are listed in the metadata are in the group, they will be notified and then can chime in if they’d like.

I understand the idea here, but the amount of manual labour required to find people and then individually ping them makes each review a lot more work. It likely won’t scale if it has to be done each time (I’m also not a fan of DMs—it doesn’t quite “default to open”, but that’s a different issue).

On the other hand, the issue with quick-docs is that they include lots of different topics. So an expert in topic A that is in the committers group and is happy to help with that bit of the quick-docs will find notifications from all the other topics quite noisy. Not a lot can be done about that bit currently since no git forge provides functions for folks to watch only parts of repositories.

I’ll start sending help requests to discourse also. Let’s see how that goes. Question about this: what category should these be sent to, and what tags should one use?

Here’s what I send to the -devel list, for example:

Yes please, that’d be great.

I was looking for work-around.

if, very big ifs, …
We build a list of subject matter expert by categories,
We map out the list with subject categories (kernel, Pipewire and so on)
We maintain the list in Pagure (start small in one Git forge),
We add additional metadata categories in Quick Docs repo that enable PR/Issues to be notified to the group,
We have a reward program for those who volunteered to be listed in subject matter expert list, and help out with PRs,

we could lessen administrative burden.

I believe Team workflows will be the right one.
We need a new sub-category like “Technical review”, or “PR review/triage”.
Use tags that match teams/SIGs or topics (amd, pipewire cockpit kernel-modules for an example).

1 Like

See also:

Just FYI folks, the quick-docs repo is now protected :placard:

Guys, we are pretty much on the wrong track right now, fiddling with symptoms or trying to cover up problems with technical measures.

We don’t have a problem in any way that we have too many commits and too few PRs. A quick look at the figures shows:

In Nov. we have 18 commits and 7 PRs. Many commits deal with small problems that do not require PRs. We had in Oct. 7 commits, in Sept. 4 commits. Even if I may have overlooked something in the numbers in a hurry, the figures are truly manageable.

If we had a group that took care of QDs, this would be absolutely no problem and no lack of discussion.

Other issues are more problematic:

We currently have 6 open PRs, open between 13 days and 3 months.
We have 18 issues, open and more or less unprocessed, between 13 days and 4 years.

I don’t see that blocking direct commits would improve anything - rather the opposite.

That gets very much closer to the problem.
This also includes the fact that we have 22 people in the committers group, but only 2 have made commits in the last 3 months. The size of the group gives the illusion of false potential and also suffers from a lack of structure and commitment. This is what we need to work on, not a technical change to the PR process.

De facto, I have been the “lead” for about 1 1/2 years. I was pretty much alone in the wide field, and I lacked and still lack a lot of knowledge about Fedora project workings. I’m happy to step back and hand over responsibility.

@ankursinha I take it for granted that from now on you will take the lead and take care of organization, planning and improvements of Quick Docs?

Hrm, this isn’t the problem we were trying to solve here. We were trying to increase visibility of changes to the repo. Direct commits are “silent” changes and I’m not aware of a way in Pagure to ensure that people that do want to keep up with changes can do so. What a PR workflow ensures is that people that are interested in keeping up with changes can do so more easily.

Is opening a PR really so much extra work that it’ll create contribution issues? Have we received feedback from community members about this to indicate so?

Sure, and these issues are not specific to quick-docs nor to docs—they are Fedora wide. We’re always short of human resources. We (and lots of folks are working) are working on increasing contribution and retention of contributors, but it has always been an uphill task.

I am very happy to continue helping with quick docs in any way I can. I help with issues, try and review PRs as soon as possible, and generally help with whatever else there is to do.

I do not want to be the “lead” in the sense that it’s usually a bad idea for any one person to be the single point of contact/failure on repos/teams.

Sorry folks, I didn’t realise the change would be so disruptive. Could someone with admin access please undo it?

I apologise profusely for jumping the gun here. I won’t let it happen again. I’m very sorry if I upset anyone at all. That was not the intention.

The change has been undone :no_entry_sign: . Sorry about that, folks.