Fix github actions to only run on either pushes on main, or PRs to main

[samuel-williams-da]

No description provided.

[shayne-fletcher]

@samuel-williams-da i'm not sure this is what we want. i need to be able to push branches and see the results of tests against those branches before i raise a pull request. this is a standard workflow for me.

[samuel-williams-da]

Can you not use a draft PR for this? Are you intending to have long running branches on this repo, vs a fork?

[samuel-williams-da]

@nycnewman What do you think, should allow CI to run on pushes to any branch?
This would mean removing the check for pull_requests, else it would run twice. Is this not what we used to have?

[shayne-fletcher]

Can you not use a draft PR for this? Are you intending to have long running branches on this repo, vs a fork?

this change applies to all repos which include my fork. my workflow is generally, branch on my fork, push, test changes (the actions are run on my fork) then open a PR (then gets tested at review time on the primary repo). this change will stop those tests from running.

[shayne-fletcher]

@nycnewman What do you think, should allow CI to run on pushes to any branch? This would mean removing the check for pull_requests, else it would run twice. Is this not what we used to have?

i don't see a problem with the tests running twice (but, see below, i also don't see under what circumstances given the standard workflow this will happen). i also do not see the downside of allowing tests to run on branches. both of these things are normal practice.

[samuel-williams-da]

The issue with running twice is simply bloat and using more CI time.
As for where CI runs, I have to defer to Edward here. It's probably fine I agree, perhaps just on: [pull_request, push] will do it.

[shayne-fletcher]

The issue with running twice is simply bloat and using more CI time. As for where CI runs, I have to defer to Edward here. It's probably fine I agree, perhaps just on: [pull_request, push] will do it.

actually, under what circumstance do you expect to see duplicate runs? in the normal flow (e.g. what's happening right now in #543) you'll see the first CI runs on the primary repo when the PR is raised and not before (and you have to approve those workflows to run). if there are subsequent pushes to the branch the PR is based on then yes, CI will run again on those changes. in summary, i don't expect to see duplicate runs.

[samuel-williams-da]

I opened a test PR earlier today which created runs for both push and pull request on every commit in the branch

[samuel-williams-da]

See the last commit on this pr

[shayne-fletcher]

Can you not use a draft PR for this? Are you intending to have long running branches on this repo, vs a fork?

for the avoidance of doubt, no i don't intend to have long running branches on the primary repo and in fact, afaik i don't have write permission to create branches on the primary repo even if i wanted to.

[shayne-fletcher]

See the last commit on this pr

ah yes, when you have a branch on the primary repo then indeed you'll see it run tests against the branch then again on raising the PR and finally again when the PR lands. indeed, this is normal.

[samuel-williams-da]

Right okay, well I think it should be as simple as changing to on: [pull_request, push], but we'll have to wait on Edward regardless for approval.

[samuel-williams-da]

See the last commit on this pr

ah yes, when you have a branch on the primary repo then indeed you'll see it run tests against the branch then again on raising the PR and finally again when the PR lands. indeed, this is normal.

The issue is that these commits were made after the PR was, so every new fix to the PR while its in progress gets CI run twice.
I think GA has a way to avoid this though, via the [] syntax above.

[shayne-fletcher]

See the last commit on this pr

ah yes, when you have a branch on the primary repo then indeed you'll see it run tests against the branch then again on raising the PR and finally again when the PR lands. indeed, this is normal.

in fact, not to belabor the point, it's what you want. when developing you want to see tests running against the branch. when you raise a PR, you want to see tests running against the target branch (master) after merging the dev branch. finally, when you land the PR, you want to see tests running against the now merged target branch.

[samuel-williams-da]

I'm not sure if github automatically rebases your branch before running CI for pull request jobs, so it may be that the pull_request and push jobs are running exactly the same thing in the case of pushing to an open PR.
Regardless, I'm not convinced the push CI has any value once a PR is open, since you only care for merging.
I'd rather we simply try to match what we had before with Azure, which is what the fix I mentioned would do.

[samuel-williams-da]

I will also say that on the daml repo, where we spend most of our time, we only run CI once a pull request it opened, which is why we had originally copied it here.

[shayne-fletcher]

Right okay, well I think it should be as simple as changing to on: [pull_request, push], but we'll have to wait on Edward regardless for approval.

i'm losing track of this discussion (sorry!) but if this is what the proposal is, then i support it. that config is standard in my experience:

on:
  push:
  pull_request:

[shayne-fletcher]

automatically rebases your branch before running CI for pull request jobs

a good point - it probably doesn't and i muddled up what happens at work in this respect 😆

[shayne-fletcher]

I will also say that on the daml repo, where we spend most of our time, we only run CI once a pull request it opened, which is why we had originally copied it here.

ah yes i follow this bit now. using the azure workflow required i open a draft PR to get testing.

i have found github actions superior in this respect in that i can run tests without having to open a PR and only when satisfied do that

(in other workflows not involving this repository in fact it enables testing of branches and merges to master that skip raising PRs entirely - not advocating that to be a reasonable practice here! 😆 )

[nycnewman]

We can proceed with on push and on pull_request and just monitor the associated costs. My main concern is around access to secrets but i understand this to be restricted for fork branches

[samuel-williams-da]

It doesn't look like having push on every branch is particularly common, I can't seem to find a way to stop the duplicate jobs, bar restricting the pull_request job to only when a PR opens.
The duplicate jobs are annoying, but not blocking - I'll leave as is for now.

[shayne-fletcher]

Can you not use a draft PR for this? Are you intending to have long running branches on this repo, vs a fork?

if we land this it just means i need to open a draft PR for testing on my fork to get tests right? that's not a big deal. shall we do it @samuel-williams-da ?

[samuel-williams-da]

I'm going to leave this for now, as the issue appears to have gone away