Skip to content

Conversation

@DiamondJoseph
Copy link
Contributor

@DiamondJoseph DiamondJoseph commented Apr 12, 2025

Removes the check workflow and using it to deduplicate PRs and pushes: CI should be run on all PRs and also on main branch: pushes to PRs are already picked up by the pull_request filtering: "By default, a workflow only runs when a pull_request event's activity type is opened, synchronize, or reopened.", the definition of synchronize can be found in the documentation: "A pull request's head branch was updated. For example, the head branch was updated from the base branch or new commits were pushed to the head branch."

E: TC believes someone at some point wanted CI to run on branches that do not have open PRs, because when a PR is opened he gets notified whether it is draft or not. This does require that now a draft PR is opened for CI to run, but does anyone have a use case for running CI against a branch without even a draft PR?

@DiamondJoseph
Copy link
Contributor Author

The behaviour that all pushes to a PR are picked up is demonstrated by me accidentally not initially removing the template's symlink to _check.yml

@DiamondJoseph DiamondJoseph requested a review from coretl April 12, 2025 16:35
@DiamondJoseph DiamondJoseph changed the title Remove check workflow and filter on branch name ci: Remove check workflow and filter on branch name Apr 14, 2025
@GDYendell
Copy link
Contributor

I'm pretty neutral on this one. What are we trying to achieve, just simpler CI or to encourage people to make PRs earlier in the development cycle?

@DiamondJoseph
Copy link
Contributor Author

DiamondJoseph commented Apr 25, 2025

Simpler CI and make the actions tab of a PR clearer to read- the skipped job seems to be run first, so every PR I make I open the wrong ci pane as it's the first one I see

image

When there could be 5+ panes of workflows (see blueapi) it's just one more piece of visual noise.

@GDYendell
Copy link
Contributor

Yeah OK, I like this change.

@gilesknap
Copy link
Contributor

I'm afraid that I'm rather attached to CI running in branches. But maybe I need to change my workflow. Being forced to make a draft PR means that I'm less likely to forget I have made some changes in a branch and not yet PRed them.

So maybe I'm convinced this is a good thing

@coretl
Copy link
Contributor

coretl commented Apr 29, 2025

Ok, if you've convinced @gilesknap and I can't remember who wanted this in the first place then I'm happy to merge this if there are no other objections. I will assume that you have tried this out somewhere and it works as expected...

@gilesknap
Copy link
Contributor

This does represent a not immediately obvious change to workflow. Should we mention this in the docs?

@DiamondJoseph
Copy link
Contributor Author

This does represent a not immediately obvious change to workflow. Should we mention this in the docs?

I've added an ADR with some rationale and the expected changes

@DiamondJoseph
Copy link
Contributor Author

I will assume that you have tried this out somewhere and it works as expected...

I've tried it out here with the additional commits to the branch, and in a personal project, where CI is run only when I expect it to.

gilesknap
gilesknap previously approved these changes May 6, 2025
Copy link
Contributor

@gilesknap gilesknap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice (ADR) approved.

@DiamondJoseph
Copy link
Contributor Author

@AlexanderWells-diamond points out that this doesn't run on tags, but the CI does not appear to currently run on tags- they could be added simply enough.

@gilesknap
Copy link
Contributor

@AlexanderWells-diamond points out that this doesn't run on tags, but the CI does not appear to currently run on tags- they could be added simply enough.

I usually make beta releases in branches. I understand that there is differing opinions on this.

@gilesknap
Copy link
Contributor

I'm happy that we can run CI on PR or tag. Approved again.

@GDYendell
Copy link
Contributor

GDYendell commented May 13, 2025

Are we are squash merging this given the merge commit?

@coretl
Copy link
Contributor

coretl commented May 14, 2025

Are we are squash merging this given the merge commit?

If no-one has adopted this elsewhere then that seems reasonable

@coretl coretl merged commit e07bbe8 into main May 14, 2025
5 checks passed
@coretl coretl deleted the deduplicate branch May 14, 2025 15:41
DiamondJoseph added a commit that referenced this pull request Jun 19, 2025
Re-fixes the issue in #253 which made modifications to only one of the
ci.yaml files and therefore only to this repository's workflows not the
workflows of generated repos.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants