chore: Try updating CODEOWNERS

[nschonni]

What was wrong/why is this fix needed? (quick summary only)

MDN URL of main page changed

Issue number (if there is an associated issue)

Anything else that could help us review it

[nschonni]

@sideshowbarker I don't think you have admin here but have merge rights. Do you want to test @peterbe question from #3680 (comment)
I think it should let you merge it unless the "System file changes" job is required, as only admin is required to override branch checks, not non-required failing jobs

[sideshowbarker]

@sideshowbarker I don't think you have admin here but have merge rights. Do you want to test @peterbe question from #3680 (comment)
I think it should let you merge it unless the "System file changes" job is required, as only admin is required to override branch checks, not non-required failing jobs

OK, I approved the PR but it remains blocked on the "System file changes" job - https://github.com/mdn/content/pull/3728/checks?check_run_id=2242127973 — and I don’t have perms to approve or bypass that. So I guess that means it’s working as expected?

[nschonni]

Thanks @sideshowbarker! can you take a snip of the button too? I know it won't be the big green merge button, but the grey one is also sometimes mergeable by non-admins

[sideshowbarker]

Thanks @sideshowbarker! can you take a snip of the button too? I know it won't be the big green merge button, but the grey one is also sometimes mergeable by non-admins

Actually, now that you say that, I see it does actually seem to be allowing me to merge it:

[peterbe]

Ok. So it seems the failing CI becomes more of a "visual warning" than anything for people with merge rights.
That's a bummer. But the list of people with merge rights is fairly small.
But it would be nice that it has to be escalated to people with admin rights.

So I'm trying this:

I don't even know if that works! It's hard for me to test since I'm an admin.

@sideshowbarker Can you return and take another look? Is it still possible for you to merge it (if you were to press the button)?

[nschonni]

I think you might need to check(mark) the particular build. There is a BUT ..., because if you have a required job with a file filter that doesn't match, you end up with a stalled PR. The only way around that is to drop the path filter, and filter inside the job (and cancel it instead) like https://github.com/github/docs/blob/main/.github/workflows/test.yml#L20. A cancelled Required job doesn't block a merge

[peterbe]

I think you might need to check(mark) the particular build. There is a BUT ..., because if you have a required job with a file filter that doesn't match, you end up with a stalled PR. The only way around that is to drop the path filter, and filter inside the job (and cancel it instead) like https://github.com/github/docs/blob/main/.github/workflows/test.yml#L20. A cancelled Required job doesn't block a merge

Yeah. It's true that if you require Some cool check and the Some cool check is "optional" (based on path:) then you get yourself into a weird state.

Yeah, the only other way to make it required is to always run it. And within you use the get-diff-content action. But ugh I don't wanna have to do that. the on.pull_request_target.path functionality is so neat.
Also, it can still be option to user-train all the people with merge access to always freak out a bit and call an admin if they see this workflow failing.

[sideshowbarker]

@sideshowbarker Can you return and take another look? Is it still possible for you to merge it (if you were to press the button)?

Yeah it looks like I can still merge it — I get the same as shown in #3728 (comment)

[peterbe]

That's a shame. I'm disappointed GitHub doesn't have the ability to "Require certain checks but only if they apply".
Perhaps they add that in the future. It kinda makes sense why the business logic isn't that sophisticated but we can only hope they implement it someday. In fact, emailed a friend at GitHub to "complain" about this.

Our list of people-with-write-access is pretty long but it seems they're all familiar and well-known peeps who would be sensible to use their merge privileges sensibly and pause to think if they see that warning:

@nschonni and I have a rough plan of how we could hack around it but we have to be smart with our total time spend. @chrisdavidmills Are you OK with leaving it as it is for now? I.e. that if a rogue PR submitter makes a rogue PR (that changes system files) that people with write-access just get the red visual warning here in the GitHub UI near the merge button.