[DEPR]: Make OSPR bot unaware of which repositories CCs can merge to

[kdmccormick]

Proposal Date

2023-02-03

Target Ticket Acceptance Date

2023-03-13

Earliest Open edX Named Release Without This Functionality

March/April 2023 (not tied to a release)

Rationale

Background

This application looks at a people.yml file for community data. Here's a sample people.yml. If you have access, here's the real people.yml. The only use people.yml currently is the list of repos that Coding Core Contributors (aka Core Committers) have access to. When a CC opens a PR, it uses this data to decide:

• do I add the core committer label or not?
• do I show a special CC OSPR message or a normal OSPR message?

Problem

Since it is no longer the source of truth for CLA data (that's salesforce-export.csv, now), people.yml is liable to falling very out of date. At time of writing this ticket, its last sync with reality was 3+ months ago, via manual update.

There are already two other places where CC repos need to updated: GitHub permissions and the CC wiki page. We (tCRIL) are not keen on keeping the repos in people.yml up-to-date unless there is compelling reason for us to do so.

Removal

#246

(this section needs updating)

• Delete people.yml: https://github.com/openedx/openedx-webhooks-data/blob/main/people.yaml.

• Remove the data structures, utility functions, and tests for people.yml from https://github.com/openedx/openedx-webhooks.

• Remove all logic from https://github.com/openedx/openedx-webhooks that pertains to Core Committer access. The only thing the bot should care about is whether a CLA has been signed.

• Remove the core committer label from labels.yml

• Optional: Reword the generic PR message:

  Thanks for the pull request, @...! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

  to something that makes sense for core contributors. For example, it might say:

  Thanks for the pull request, @...! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Of course, Core Contributors to this repository may merge PRs themselves
  ...

Replacement

Core Contributors can manually add the core contributor label to their PRs with label: core contributor.

There is no other planned replacement. However, here are some alternatives if we wanted to keep the bot aware of core committer repo access:

• Have the OSPR bot use the GitHub permissions API rather than people.yml to determine whether someone has write access to a repository.
• Automatically sync CC repository access information into people.yml. For example, maybe we could store CC access information in Salesforce, and then percolate down to both GitHub and this application. I don't really know how this would be done.
• Have tCRIL on-call manually keep people.yml in sync with GitHub and the wiki.

Deprecation

No response

Migration

Before accepting this deprecation

Get consensus from @feanil , @sarina , @nedbat , and @mphilbrick211, and @e0d that this change makes sense. I am aware that this change might not make sense, but I felt like a DEPR ticket was the best way to discuss it.

After accepting the deprecation

Inform CCs in Slack and on the forums about the workflow changes. Ned could send an email to 2U folks to inform them that PRs from CCs and non-CCs will now look identical.

Additional Info

No response

[nedbat]

The "core committer" label is also a signal to other reviewers that the author has a certain status in the community. Can we get that some other way?

[nedbat]

BTW, the core committer information is currently the only useful information in the people.yaml file, so if we move ahead with this I think we can completely remove people.yaml code from the bot, and simply abandon the people.yaml file.

[kdmccormick]

Oh, good to know @nedbat . Where does the CLA info come from?

[nedbat]

Oh, good to know @nedbat . Where does the CLA info come from?

From the salesforce export, https://github.com/openedx/openedx-webhooks-data/blob/main/salesforce-export.csv (private link).

[kdmccormick]

Good to know, thanks. I updated the issue description.

[itsjeyd]

@kdmccormick

Removal

...

• Remove the core committer label from labels.yml

To clarify, would that mean we'd completely retire the core committer label? Or would it still be possible to manually add it to PRs authored by CCs?

CC @mphilbrick211

[kdmccormick]

@itsjeyd There's no technical reason why the core committer label couldn't be added manually by anyone with write access to the repo (which, of course, would include that repo's core committers).

@mphilbrick211 , how do you feel about the core committer label no longer being added automatically? Also, would you want to still let CCs add the label to their own pull requests manually, or would you rather get rid of the label altogether?

[sarina]

Now that Feanil has done his repo conformance work, anyone can type label: label name as a comment on a PR or issue and apply that label

[sarina]

label: core committer

[kdmccormick]

I've asked the CCs to weigh in on Slack: https://openedx.slack.com/archives/C015KC8CN76/p1678117838078079

[nedbat]

label: xyzzy

[kdmccormick]

This is now Accepted. In the coming weeks I will communicate the change to core contributors and to 2U and then remove this functionality from the OSPR bot.

[kdmccormick]

Sorry, I forgot about this for a bit!

I'm going to push through openedx-unsupported/terraform-github#61 first, which will add the core contributor label to every repo. Then, we can disable the core committer label, and explain to everyone at once the changes in label behavior.

[itsjeyd]

That sounds great @kdmccormick, thanks for the update!

[kdmccormick]

I talked with @mphilbrick211 and @itsjeyd about the new core contributor label. They said that they'd rather have that label be added (manually or automatically) to every PR which a Core Contributor makes, whether or not the CC has merge rights on the particular repository that the PR is against. They explained that this would be useful to them because Core Contributors tend to proactively manage their own PRs (regardless of the repo) whereas non-CC PR authors tend to need more attention & help.

Obviously, these semantics would be easier to automate than the current core committer label semantics.

So, the question on my mind on now is: should we

1. have the OSPR look at salesforce-data.csv and automatically add core contributor where appropriate, or
2. stick to the original decision and have core contributor be manually applied by authors & project managers?

@nedbat , let me know if you have an opinion in either direction.

[kdmccormick]

@nedbat @feanil After further consideration, I'm coming to the conclusion that this DEPR as I wrote it is overzealous. At least in its current form, the OSPR bot needs to remain aware of who is a CC on what repo, even if we switch to the new core contributor labelling semantics described in my previous comment.

Why? Putting aside the core committer label, the OSPR bot needs to know whether a user is a CC on a particular repo because it affects whether a 2U Jira ticket is created:

• if the user is "internal" (to 2U or Axim) then openedx-webhooks doesn't get involved.
• if the user is a "external" but a CC on a repo, then the bot doesn't create a 2U Jira ticket, because 2U doesn't need to get involved in order to merge the PR.
• otherwise, a Jira ticket is created.

Now, this flow is partially broken currently because we are reading the CC data from the no-longer-updated labels.yaml, which becomes more and more outdated every time a coding CC is added or removed.

In order to fully fix this flow we would need to do a few things:

• Publish CC Data In salesforce-export.csv #207
• Pull Org Data from the Export CSV instead of orgs.yaml #199
• Use ownership data to determine whether something is an "ospr" #190

Of course, this is assuming that 2U and the Open edX project managers want to continue using the Jira OSPR flow, which is something I don't feel equipped to make decisions about.

In the meantime, I can still carry out the part of this DEPR that is "stop using the core committer label", but the Jira ticketing flow will still be off. I am happy to remove the core committer label but I can't go any further than that now.

[nedbat]

As of be68232, the bot has no information about who is a core contributor, and does nothing special for them.

Do we want to keep this issue for the future work of bringing this back when the information is in the Salesforce export?

[kdmccormick]

Thanks Ned. Let's close this since the deprecated feature has been removed, and use #207 for the future work.