Autotester: allow testing of external contributions w/o approval

[mayrmt]

Enhancement

@trilinos/framework

I'm putting out this idea for discussion.

Current state and problem description

Currently, PRs submitted by external developers (i.e. not a member of the GitHub Trilinos organization) are only tested by the autotester, if they have been approved. However, this often leaves them in this weird state of being approved "just for the sake of testing", but without an actual code review. In theory, they could be merged "unreviewed".

Examples:

• 9184 winllvmparbuild: fix winllvm parallel builds with modification to muelu, zoltan2, belos #9187 (review)
• MueLu: interpolation order input changes for GeometricInterpolation #9153
• MueLu: Add SemiCoarsenPFactory_kokkos #9107 (review)

I can definitely understand, that one wants to somehow control, which/how many external PRs are tested. However, requiring approval just to trigger the tests seems weird to me as outlined above.

I also see value in starting testing early on, as some problems only become evident during testing and then often require several iterations of code changes and testing.

Possible solution

As far as I know, labels can only be applied by members of the Trilinos organization. So, instead of requiring approval, one could trigger the autotester for external PRs by setting a label "AT: ready for testing". This would still require some action of a Trilinos member to trigger testing, hence uncontrolled overload of testing machines by external PRs can be avoided as well. However, testing would be possible without premature PR approvals.

[jhux2]

@trilinos/trilinos-product-owners @ccober6

[mayrmt]

Here's another example: PR #9259

[jhux2]

As far as I know, labels can only be applied by members of the Trilinos organization.

Is this true? I've seen users add the question label.

So, instead of requiring approval, one could trigger the autotester for external PRs by setting a label "AT: ready for testing". This would still require some action of a Trilinos member to trigger testing, hence uncontrolled overload of testing machines by external PRs can be avoided as well.

[mayrmt]

@jhux2 asked:

Is this true? I've seen users add the question label.

I think so. I just had my colleague @maxfirmbach (not a member of Trilinos) double check. He has created PR #9153, but he can't add labels to it. Specifically, he cannot see the little gear symbol next to labels on the PR website, while I can. So, seems that there's a difference.

[jhux2]

@mayrmt The framework team has implemented this option. It's called AT: PRE-TEST INSPECTED. My understanding is that merging a PR from someone who isn't a member of Trilinos is now a two-step process:

1. Apply the label AT: PRE-TEST INSPECTED to initiate testing.
2. Formally review the PR. This alone does not start testing , you need step 1).

For Trilinos members, the workflow is unchanged.

[mayrmt]

@jhux2 Thanks for following up on this!

I'll leave this issue open, until I have tried that process. Then, I'll close.

[mayrmt]

I've seen the label AT: PRE-TEST INSPECTED in action quite some times, so let's close this issue. Thanks to @trilinos/framework for setting this up!

[mayrmt]

In PR #10437, I'm seeing that the autotester asks for the AT: PRE-TEST INSPECTED label even after the PR has been approved (see this comment by the attester: #10437 (comment)). Is this an intended behavior? I'd say that approval from code owners should include the pre-test inspection, right?

[e10harvey]

Is this an intended behavior?

Yes.

I'd say that approval from code owners should include the pre-test inspection, right?

This is pending in the next auto-tester release.

[github-actions]

This issue has had no activity for 365 days and is marked for closure. It will be closed after an additional 30 days of inactivity.
If you would like to keep this issue open please add a comment and/or remove the MARKED_FOR_CLOSURE label.
If this issue should be kept open even with no activity beyond the time limits you can add the label DO_NOT_AUTOCLOSE.
If it is ok for this issue to be closed, feel free to go ahead and close it. Please do not add any comments or change any labels or otherwise touch this issue unless your intention is to reset the inactivity counter for an additional year.

[mayrmt]

This comment states that some changes to the auto-tester are pending. Not sure, if they have already been done. Let's keep this issue open until this has been verified.

[github-actions]

This issue has had no activity for 365 days and is marked for closure. It will be closed after an additional 30 days of inactivity.
If you would like to keep this issue open please add a comment and/or remove the MARKED_FOR_CLOSURE label.
If this issue should be kept open even with no activity beyond the time limits you can add the label DO_NOT_AUTOCLOSE.
If it is ok for this issue to be closed, feel free to go ahead and close it. Please do not add any comments or change any labels or otherwise touch this issue unless your intention is to reset the inactivity counter for an additional year.

[mayrmt]

Still an open topic...

[e10harvey]

CC: @sebrowne, @triple3567

[jhux2]

@mayrmt Approval doesn't imply pre-test inspected. The latter is explicitly needed in order for the autotester to run on a PR authored by someone who isn't in the Trilinos organization.