[GH Request] Can edx-transifex-bot not require PR approval for credentials, credentials-themes?

[deborahgu]

Firm Name

2U

Urgency

Low (2 weeks)

Requested Change

Can edx-transifex-bot not require PR approval for credentials, credentials-themes?

Reasoning

The jenkins jobs for pull translations for credentials and credentials-themes fail every night, and the reason that they do is that the process is:

1. Create a PR
2. Successfully passed all of CI
3. Sit around and wait for approval
4. Timeout after five minutes
5. Close the PR unmerged

This is obviously counterproductive.

I can see that we have a bot set up to not require PR approval on every repository, because if I look at the bot's PRs, they are clearly merging without requiring approval on several repos, including edx-platform, edx-analytics-dashboard, and edx-proctoring.

That being said, they are failing on the majority of repositories, so maybe this is a conscious decision. If there is, could you tell me that as well? Because then I will put in a request to find out if we can change the Jenkins job so it doesn't try to close the pull request on failure. (at least, for appropriately flagged jobs.)

Thanks.

[openedx-workflow-automation]

Thank you for your report! @openedx/axim-oncall will triage within a business day. Simple requests usually take 2-3 business days to resolve; more complex requests could take longer.

[kdmccormick]

Thanks for the report @deborahgu , I'll take a look soon.

[kdmccormick]

(Copying this in from our Slack channel where I'd originally posted it. tl;dr -- I'm going to give the bot admin rights on a repo, and confirm that this lets it merge PRs without approval)

Background:

• edx-transifex-bot is built to open PRs and merge them if they pass status checks. No human intervention required.
• This process will be removed as part of OEP-58 but we're not there yet.
• We recently demoted edx-transifex-bot's from "admin" to "write" on on most repos as part of a general permission scope-down.

Problem:

• Some repos block PR merges until 1+ people with write access have approved the PR.
• Without admin access, edx-transifex-bot is failing to bypass that requirement, and is just closing its PRs on those repos (example).
• GitHub does not allow us to example particular users (or bot users) from branch protection rules.

Proposed solution:

• Give the bot admin access back on all necessary repos until OEP-58 is done.
• Eventually, revoke admin rights from the bot as repos switch to use OEP-58.

Rejected solutions:

• Remove the approving-review requirement from repos that have it. (This is a good requirement and we want to support it.)
• Change the translation process to require human intervention--approving the PR. This would be an annoying change to make, especially now since we're so close to switching over to OEP-58.

[kdmccormick]

Huh, the bot is already an admin on credentials and credentials-themes.

@deborahgu , I see that the PR approval requirement is an issue for the bot in some repos, but are you seeing anything in the credentials and credentials-themes jobs in particular that's making it look like the PR approval requirement is the culprit? Looking at this recent credentials PR, I see "failing message compilation prevents this PR from being automatically merged" as well as this failed CI job.

Is it possible that either of those, not PR approval, is what's blocking the merge?

[deborahgu]

absolutely could be something else. When I was trying to debug it, I was looking more at credentials-themes PRs like this one, where as far as I can tell every one of the checks passed, and the only failures I could see were that it wasn't approved

My other bit of evidence is that my current work around is:

• Start the Jenkins job and wait for it to create the PR
• once the PR exists, go in and approve it by hand
• with the Jenkins job finishes, it merges correctly

So that was my source of evidence that there was an approval required. Because when I added an approval, the job passed. But it's a weird situation that I'm not saying I'm definitely right!

Could it be that the repositories are not set up for the bot with admin rights to be able to merge without approval?

[kdmccormick]

Your evidence for credentials-themes seems sound to me :) I agree that openedx/credentials-themes#703 looks like it failed to merge due to the approval requirement. I am actually seeing a difference between credentials and credentials-themes settings ("Allow specified actors to bypass required pull requests") which I'd previously thought was unrelated but might actually have an effect here.

Have you noticed the same behavior for credentials, or just for credentials-themes?

[deborahgu]

confession time: I did my debugging on credentials-themes, and then I said to myself, said I, "well that would explain both situations!"

And even though I know that sometimes we actually do have failing checks, I didn't even look to see if there had been any, so it's possible that it is unrelated.

[kdmccormick]

No problem haha, it was a reasonable assumption.

I have just updated credentials-themes' settings to match credentials'. Are you able to re-run the credentials-themes job manually and see if the PR goes through?

[deborahgu]

I did just rerun it but there were no new translation strings so it didn't make a PR. But I'm sure there will be some soon so we could hold off and wait. (I suppose we could also translate something but I don't have transifex access, heh.)

[kdmccormick]

Gotcha. I just made the same fix to edx-ora2 settings, let me reach out to Aurora and see if they can re-run that job.

[kdmccormick]

Now edx-ora2 is "failing message compilation" too :P openedx/edx-ora2#2071

I think we have two potentially-distinct issues which we need to tease apart:

1. branch protection preventing the pr from merging
2. something unknown is causing message compilation to fail

@deborahgu , could you post some logs from the credentials job so I can dig into what issue (2) might be?

[kdmccormick]

(semi-related: #940)

[kdmccormick]

Closing this as a dupe of #940

[kdmccormick]

This is all set on edx-platform now but I need to come back and make sure the same fix is applied to credentials and other repos.

[sarina]

@kdmccormick is this still in progress?

[kdmccormick]

credentials is looking good now: openedx/credentials#2325

credentials-themes hasn't gotten a translations PR since Oct 2023, so I think it is failing in the Jenkins job before it even gets to the PR-making stage. @deborahgu , I'm going to close this now, but if you fix the Jenkins job and find that PRs still aren't auto-merge, feel free to re-open this.