search

trilinos / Trilinos

Primary repository for the Trilinos Project
https://trilinos.org/
Other
1.18k stars 559 forks source link

Autotester: allow testing of external contributions w/o approval #9197

Open mayrmt opened 3 years ago

mayrmt commented 3 years ago

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:

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 commented 3 years ago

@trilinos/trilinos-product-owners @ccober6

mayrmt commented 3 years ago

Here's another example: PR #9259

jhux2 commented 3 years ago

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 commented 3 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@jhux2 Thanks for following up on this!

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

mayrmt commented 2 years ago

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 commented 2 years ago

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: https://github.com/trilinos/Trilinos/pull/10437#issuecomment-1109915030). Is this an intended behavior? I'd say that approval from code owners should include the pre-test inspection, right?

e10harvey commented 2 years ago

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[bot] commented 1 year ago

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 commented 1 year ago

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[bot] commented 2 months ago

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 commented 1 month ago

Still an open topic...

e10harvey commented 1 month ago

CC: @sebrowne, @triple3567

jhux2 commented 1 month ago

@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.