Suggested github action change, to prevent clutter in draft PRs

[morrowcj]

Prevents the full suite of github actions from running on draft PRs. This will reduce the clutter of PR timelines and notifications by preventing the automated bot actions from occurring on every push before the PR is ready. The new job in "combined_format_lint_test_mypy.yml" runs and fails when the PR is a draft, and the other jobs are skipped, indicating that the PR must be converted to "Ready for review" (which will trigger the other jobs) to proceed.

This change does not affect standard "Ready for review" PRs, which will run the jobs each time a PR undergoes a status change .

What

• A new job that fails and skips the other jobs on draft PRs
• run all jobs when the PR is converted to "ready for review"

Why

• to prevent clutter in PR timelines (and notifications) before they are ready for higher scrutiny

Test plan

• tested on this PR (see timeline)

Context

The inspiration comes from this thread.

[github-actions]

Current Coverage: 99%

Mypy errors on no-draft-actions branch: 1191
Mypy errors on dev branch: 1191
No difference in error counts

[github-actions]

🚨 Please update the changelog. This PR cannot be merged until changelog.md is updated.

[morrowcj]

According to the above time line, this did not work as expected. Although the new job immediately failed, the other jobs proceeded to run anyway. I think this is a syntax error, so I'll try to fix now.

[morrowcj]

Adding needs: fail_if_pull_request_is_draft to the format_code job causes it (and the subsequent jobs that depend on it) to be skipped.

This was successful, according to the time line and actions log:

I will now convert to "Ready for Review" to ensure that the additional jobs are run.

[morrowcj]

This didn't work, since the first check was skipped - converting back to a draft to try fixing. I will convert back to "Ready for review" once I think I've fixed it.

[github-actions]

Current Coverage: 99%

Mypy errors on no-draft-actions branch: 1191
Mypy errors on dev branch: 1191
No difference in error counts

[github-actions]

🚨 Please update the changelog. This PR cannot be merged until changelog.md is updated.

[github-actions]

Current Coverage: 99%

Mypy errors on no-draft-actions branch: 1191
Mypy errors on dev branch: 1191
No difference in error counts

[github-actions]

🚨 Please update the changelog. This PR cannot be merged until changelog.md is updated.

[morrowcj]

That worked, the draft job skipped, the format job passed, and the type checking job failed:

I will now update the changelog, and reconvert to draft then ready for review one more time as a final check.

[morrowcj]

On second thought, I do not believe this PR warrants a changelog entry, since it does not alter the actual code base.
So I'm going to convert to ready, and request review.

[github-actions]

Current Coverage: 99%

Mypy errors on no-draft-actions branch: 1191
Mypy errors on dev branch: 1191
No difference in error counts

[github-actions]

🚨 Please update the changelog. This PR cannot be merged until changelog.md is updated.

[github-actions]

Current Coverage: 99%

Mypy errors on no-draft-actions branch: 1191
Mypy errors on dev branch: 1191
No difference in error counts

[github-actions]

🚨 Please update the changelog. This PR cannot be merged until changelog.md is updated.

[PooyaHekmati]

Thanks @morrowcj for noticing and working on this.
Besides the technicalities, I would like to hear from other team members how do they feel about this. on one hand we have the benefit of reduced notifications; on the other hand, we have the benefit of making sure everything passes on github before marking the PR ready for review. we have had cases in the past that checks passed locally but failed on github.

Side note: all PRs need an entry in changelog. The decision about the importance of each entry is made when we are reading the changelog and consuming its content.

[morrowcj]

on one hand we have the benefit of reduced notifications; on the other hand, we have the benefit of making sure everything passes on github before marking the PR ready for review. we have had cases in the past that checks passed locally but failed on github.

That makes sense to me, and I agree with the pros and cons you stated. I'm interested to hear what others think!

Side note: all PRs need an entry in changelog. The decision about the importance of each entry is made when we are reading the changelog and consuming its content.

Good to know: done!

[github-actions]

Current Coverage: 99%

Mypy errors on no-draft-actions branch: 1191
Mypy errors on dev branch: 1191
No difference in error counts

[morrowcj]

Feedback from a few members of the dev team at yesterday's Work Together session seemed generally positive to this idea. One additional suggestion that came up is possibly including an option to manually trigger the jobs that are skipped on a draft branch. This can likely be accomplished with the workflow_dispatch event.

[morrowcj]

After a quick commit attempting enable this functionality and re-converting to a draft, I have confirmed that the changes were successful. The actions could be manually triggered even though the PR is a draft:

Triggering the run:

results of the run:

The only downside of this is that the results of a manually triggered run do not show up on the PR timeline.

[github-actions]

Current Coverage: 99%

Mypy errors on no-draft-actions branch: 1191
Mypy errors on dev branch: 1191
No difference in error counts

[github-actions]

Current Coverage: 99%

Mypy errors on no-draft-actions branch: 1191
Mypy errors on dev branch: 1191
No difference in error counts

[ew3361zh]

I think this is close for me, I like the direction overall and agree with the premise behind the changes.

I think working on clarity for new (and existing) contributors about what's happening with GH actions on a draft branch would be my preference. And I think if possible that should happen through the GH actions (i.e. they shouldn't fail here but rather not run)

[ew3361zh]

Can you talk a little about how you put together this particular group of types (and maybe what synchronize is?)?

[morrowcj]

Great question: the defaults (if you don't include the "types" key) that trigger an action on PR are opened, synchronize, and reopen. I wanted to also add triggers for converting the PR to and from drafts. Source

I believe synchronize is whenever there is a commit pushed to the HEAD reference of the PR's "from" branch.

[ew3361zh]

I realize the purpose of this is to declutter the conversation on a PR by not posting unnecessary messages but just wondering if you think it would be confusing for contributors (new/old) to see actions failed with no explanation rather than just not running them at all if it's a draft PR?

I think I'd probably prefer just not running the jobs without explicitly failing.

[morrowcj]

I think I agree with you and was originally going to do exactly that (skip checks on draft) but came across a thread where people expressed interest in being alerted or otherwise indicated that the skips occurred in order to remind them that they need to flag it as ready for review. If we don't have that need, I'm happy that remove the failing check!

[github-actions]

Current Coverage: 99%

Mypy errors on no-draft-actions branch: 1191
Mypy errors on dev branch: 1191
No difference in error counts

[github-actions]

Current Coverage: 99%

Mypy errors on no-draft-actions branch: 1191
Mypy errors on dev branch: 1191
No difference in error counts

[morrowcj]

@ew3361zh, I removed the check that fails on a draft PR. I then converted back to a draft to test and, as you can see, the tests were successfully skipped:

and then fully ran when converted back to "ready for review":

I also replied to your question about the types above and will re-request your review.

[morrowcj]

I also wonder if it is possible to reduce the frequency of redundant comments like these ones:

Perhaps by having the bot edit the comment if it already exists instead of creating a new one. It seems that option exists, but probably better suited for a different issue/PR.