stacks-core icon indicating copy to clipboard operation
stacks-core copied to clipboard

Avoid extraneous CI runs

Open obycode opened this issue 1 year ago • 6 comments

Our ci.yml currently includes the following trigger:

  pull_request_review:
    types:
      - submitted

This causes the CI to be rerun whenever a review is submitted. This is not needed and causes unnecessary delays and runner usage. The jobs are already guaranteed to be re-run when anything changes based on the pull_request triggers:

  pull_request:
    types:
      - opened
      - reopened
      - synchronize
      - ready_for_review

obycode avatar May 03 '24 16:05 obycode

i'm kind of thinking here that we should only trigger on

  • synchronize
  • ready_for_review

i'm going to try a few options here, then PR against next

wileyj avatar May 06 '24 20:05 wileyj

I think I would lean toward "opened" and "synchronize". Removing "opened" and keeping "ready_for_review" would not allow us to check the test status before asking for reviews, and that seems like a good practice. Unless we're really concerned about using too much runner time on "not ready" PRs, then I could understand that change.

obycode avatar May 06 '24 20:05 obycode

I think I would lean toward "opened" and "synchronize". Removing "opened" and keeping "ready_for_review" would not allow us to check the test status before asking for reviews, and that seems like a good practice. Unless we're really concerned about using too much runner time on "not ready" PRs, then I could understand that change.

yea, i see where you're coming from and that's a good point. no reason to review if the expected tests are failing

wileyj avatar May 06 '24 20:05 wileyj

@obycode i found an interesting edge case that might cause some problems without further changes.

consider the case where a branch is PR'ed where the only change is to CHANGELOG.md or some other non-code file (currently configured to ignore md and yml files). In this scenario, CI will never run - but the status checks are still enabled which would require either a small "code" change (like an extra comment/line in a file), or an override to merge a PR (which i woild prefer avoiding).

i'm not sure if there's a way to disable status checks based on what is changed in the PR, leaving the only other option being that we trigger workflows on any PR change, markdown files included.

any thoughts on this?

wileyj avatar May 08 '24 16:05 wileyj

actually, i think i have my answer - it's possible now, but will have to come in a future PR. for now i'll simply remove the ignored paths trigger:

    paths-ignore:
      - "**.md"
      - "**.yml"

wileyj avatar May 08 '24 16:05 wileyj

next: https://github.com/stacks-network/stacks-core/pull/4764 develop: https://github.com/stacks-network/stacks-core/pull/4765 master: https://github.com/stacks-network/stacks-core/pull/4766

wileyj avatar May 08 '24 16:05 wileyj