attempt a nightly workflow only on approved PRs

[gidden]

Required: write a single sentence that describes the changes made by this PR.

Copies nightly workflow, changes to evoke on pull_request_review, and checks that review state is approved.

Related to #591.

How to review

Required: describe specific things that reviewer(s) must do, in order to ensure that the PR achieves its goal. If no review is required, write “No review:” and describe why.

Approve the PR, and confirm that the nightly tests are executed and result as expected.

PR checklist

• [ ] Continuous integration checks all ✅
• [ ] Add or expand tests; coverage checks both ✅
• [ ] Add, expand, or update documentation.
• [ ] Update release notes.

[codecov[bot]]

Codecov Report

Merging #603 (764d5a9) into main (b572cce) will not change coverage. Report is 412 commits behind head on main. The diff coverage is n/a.

Additional details and impacted files

@@          Coverage Diff          @@
##            main    #603   +/-   ##
=====================================
  Coverage   92.8%   92.8%           
=====================================
  Files         42      42           
  Lines       3287    3287           
=====================================
  Hits        3052    3052           
  Misses       235     235           

[gidden]

For future reviewers/discussion. This PR has successfully invoked the nightly tests after approval.

@iiasa/messageix-devs should we place this in review? If so, who should review? If not, how should we proceed?

[LauWien]

For future reviewers/discussion. This PR has successfully invoked the nightly tests after approval.

@iiasa/messageix-devs should we place this in review? If so, who should review? If not, how should we proceed?

If this is the way we want to go, I could review :)

[khaeru]

Thanks @gidden for the proof-of-concept! I edited the description to add a back-link to the issue that prompted this. Continuing discussion here so that the issue can focus on the specific issue with the nightly test scenarios.

We need to think here about what the workflow looks like.

1. Author opens a PR with some commits. Status in our tracker is "In progress".
2. Author develops their PR, pushing commits, noting and ensuring that the "pytest" and "lint" checks pass.
3. Author requests a review; status is "In review".
4. Reviewer does the review, possibly requesting changes.
5. Review and author interact until reviewer is satisfied.
6. Reviewer approves.
7. (New) This new workflow runs.
8. Possibly (as in this case) the workflow fails.

What happens next? The PR is "approved", status is "In review", but checks are failing, and it is not actually ready to merge.

Instead (I guess) the author needs to go back to work (step 2), reset the status to "In progress", and then the process repeats. During this process, the author may need to check multiple (many) times that their fixes will allow the nightly tests to pass, i.e. they will need to run those nightly tests, either locally or on CI. If the only way to do this (recalling that authors cannot review their own PRs) is to ask the reviewer to come give a "not-actually re-approval," this becomes more demanding on the reviewer and may slow action.

[khaeru]

I think I'd favour a process more like this:

1. Author opens a PR with some commits. Status in our tracker is "In progress".
2. Identifying that their PR touches the GAMS implementation, author pushes a commit uncommenting these lines: https://github.com/iiasa/message_ix/blob/b572ccee45c293a88f1f2acf91a0811d50cf6593/.github/workflows/nightly.yaml#L6-L8 with a message like TEMPORARY Enable nightly workflow for PR.
3. Author develops their PR, pushing commits, noting and ensuring that the "pytest", "lint" and "nightly" checks pass.
4. Author requests a review; status is "In review".
5. Reviewer does the review, possibly requesting changes.
6. Review and author interact until reviewer is satisfied.
7. Reviewer approves.
8. Author or maintainer performs git rebase -i on the branch to drop the "TEMPORARY" commit from step 2.
9. Merge.

The reasons:

• The author can do this unsupervised; it is not technically difficult.
• We avoid the need to have a second person (the reviewer) repeatedly in the loop just to invoke the new workflow.
• An "approve" review continues to mean "the PR is ready to merge", not "we are just trying to poke the nightly tests to see if they will now pass."
• We don't need to remind/automate moving PRs back to the "In progress" state in the tracker.

[LauWien]

@iiasa/messageix-devs should we place this in review? If so, who should review? If not, how should we proceed?

Ah sorry for my comment, I didn't get your question! :)

[OFR-IIASA]

Having now tested this feature in a PR, I must say I like the workflow suggested by Matt.