[GitHub Request] Allow maintainers to bypsss "Require status checks to pass" for OpenSearch-Dashboards

[joshuali925]

What is the type of request?

Settings Update

Details of the request

This is the same request as #348, creating a new issue since I cannot reopen the old one.

Our ask was

1. Enable "Require status checks to pass" for GitHub Actions starting with Build and Verify, Run cypress tests, and Lint and validate. The target branch is main and the version branches.
2. Maintainers should be in the "Bypass list" if not already added.

Item 1 appears to be completed.

For item 2, as part of the testing guide, we agreed that maintainers should be able to override the failed actions

The workflow action should pass before PR is merged, unless maintainers override them with a comment.

Maintainer permission is defined as

.github/RESPONSIBILITIES.md

Line 38 in bb5391a

Maintainers are active and visible members of the community, and have [maintain-level permissions on a repository](https://docs.github.com/en/organizations/managing-access-to-your-organizations-repositories/repository-permission-levels-for-an-organization). Use those privileges to serve the community and evolve code as follows.

But right now I'm a maintainer, I should have maintain-level permission, yet I'm not able to override failed actions.

Is it possible to add maintain level permission to the bypass list as mentioned in item 2? This is what it should look like

If it's not possible, please explain a bit with supporting references. Thanks!

Additional information to support your request

#348

When does this request need to be completed?

not urgent

Notes

Track the progress of your request here: Engineering Effectiveness Board (view).
Member of @opensearch-project/admin will take a look at the request soon.
Thanks!

[gaiksaya]

Thanks for raising the request @joshuali925
Looking for some guidance on the best practices followed in the open source world.
Maybe @msfroh @dblock @andrross can help.
Thanks!

[msfroh]

I'm not a huge fan of letting maintainers bypass required status checks. Status checks are required or they're not. Since maintainers are required to merge PRs anyway, letting them merge even when required status checks are failing is only adding one extra click to the process. I don't understand what problem is solved by making the status checks required.

As linked by @joshuali925, the OpenSearch Dashboards testing guide documents this process, and I personally have no issue with GitHub behavior matching the documented process. I also have no real skin in the game, though, as I don't work on or use OpenSearch Dashboards. If you want my advice/opinion/guidance, the process sounds like a very bad idea.

[joshuali925]

@msfroh thanks for sharing your advice, I do agree ideally if we require status checks, we should require them without exceptions.

But realistically I sent a PR a while ago to modify a comment, and codecov reported coverage increase opensearch-project/OpenSearch-Dashboards#4695 (comment). I have also seen comment change led to inaccurate coverage decrease from codecov. The reason we still want to allow bypass is that we do not want to block urgent PRs on CI stability problems if we know they are not relevant.

The other reason is that the OpenSearch-Dashboards repo did not require any status check before. Having the override option reduces friction to the change and accounts for unexpected issues, while it still makes maintainers to think twice. We do plan to have internal tracking mechanisms and revisit each manual override. I personally want to raise another proposal if codecov and other CIs appear to be stable over time and remove the override option.

Although feel free to suggest alternatives. I have considered reaching to this repo and admins when we need overrides, but it doesn't sound to be a good practice, because admins probably will not have context on PRs for OpenSearch-Dashboards, and

Requests typically require 7-10 business days to process

[msfroh]

Got it!

That makes sense, especially during the transition period as the bugs get worked out.

On OpenSearch core, we've opted to leave the coverage check optional -- that's one where we expect maintainers to exercise some judgement. I personally consider it to be an informative check -- before I comb through a PR, I get a sense of whether tests were written yet or not.

[peterzhuamazon]

I think make coverage optional might be a good option as well under certain circumstances.
Altering default behavior of maintainers could introduce more issues.

Thanks.

[andrross]

Code coverage is a useful but certainly imperfect metric. I also recommend not making that a required check. Coverage requirements can lead to developers writing useless tests just to boost coverage*. I also agree in general that if a check isn't truly required in that there is some established process for maintainers to skip the check, then it should just be marked as not required as opposed to allowing overrides.

*For example, in the Java world sometimes you write a class to hold only static methods. You give it a private constructor because it's never intended to allow instances to be created, but now code coverage says that constructor is untested, so a dev might make the constructor visible just to be able to reach it in tests, but now we've got unnecessary code in both the class and the tests just to appease the coverage metric!

[joshuali925]

@andrross I still think that making coverage not decrease is important. GitHub supports the override feature for a reason. To me as a maintainer, I would think twice if my PR did not satisfy the required coverage requirement before overriding, whereas if the CI is not required, I wouldn't care that much.

developers writing useless tests just to boost coverage

This happens more if we are demanding 100% coverage. The new requirement for OpenSearch-Dashboards is 80%, which is not that hard to reach properly (originally 70% but raised to 80% after internal review).

You give it a private constructor because it's never intended to allow instances to be created, but now code coverage says that constructor is untested

I don't think that's a valid example though. I've ran into similar coverage missing problems in SQL (it requires 100% coverage). In the right scenario, adding an empty private constructor increases coverage because they are excluded, and I think if the constructor contains logic, it should be tested in some way. "so a dev might make the constructor visible just to be able to reach it in tests" is a problem in the implementation that should be caught in code review, but it's not an unnecessarily required coverage problem.

I agree coverage is not the best thing to rely on, but it's the only visible mechanism on the status of tests. If we don't enforce it, we can only rely on good intentions from maintainers.

@peterzhuamazon could you elaborate on the possible issues?

[virajsanghvi]

Just want to +1 @joshuali925's assessment - coverage isn't the perfect measure, but given where OSD is at and the strong desire to prevent regressions and increase testing, its one of the best levers we can pull. We can always exclude code from coverage where appropriate (and that configuration is code reviewed).