Skip to content

[CI] Changed the Workflow Triggers #2589

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

AlexandreSinger
Copy link
Contributor

Changed the workflow triggers for the test and container workflows such that they only run when there is a push to master.

Also added concurrency such that if a CI was called on a particular PR or branch that is already running the CI, it will cancel the old run. This will prevent useless runs of the CI.

Changed the workflow triggers for the test and container workflows such
that they only run when there is a push to master.

Also added concurrency such that if a CI was called on a particular PR
or branch that is already running the CI, it will cancel the old run.
This will prevent useless runs of the CI.
@github-actions github-actions bot added the infra Project Infrastructure label Jun 5, 2024
@AlexandreSinger
Copy link
Contributor Author

@vaughnbetz I think we should merge this in. Currently the CI is full again, this time its someone's PR:
image

This is exactly what the concurrency argument is trying to resolve. I will kill these runs since I don't think this person intends on calling the CI 10 times for the same PR...

@vaughnbetz
Copy link
Contributor

Thanks Alex!

@vaughnbetz vaughnbetz merged commit 268103b into verilog-to-routing:master Jun 7, 2024
53 checks passed
# See: https://docs.github.com/en/actions/using-jobs/using-concurrency
concurrency:
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
cancel-in-progress: true
Copy link
Contributor

Choose a reason for hiding this comment

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

@AlexandreSinger This cancels previous CI runs for the master branch. I think we should avoid canceling these runs on master because we don't push to master as often, and multiple pushes are usually from multiple PRs merged in a short time. If something goes wrong, having CI results for each push to the master branch helps us pinpoint issues more easily. What do you think about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually did this on purpose since it is a common scenario that we would merge 3-4 PRs at one time. This causes a storm of traffic on the CI when this occurs which can cause problems. Above this section, I specifically allowed the main branch to trigger the CI on push since, I agree, that the main branch should test on merge. I personally just find running the CI on every PR merge a bit much.

If we decide to allow the CI to trigger on every push to main (and run concurrently), this resource discusses how to do that: https://docs.github.com/en/actions/using-jobs/using-concurrency#example-only-cancel-in-progress-jobs-on-specific-branches

Its very simple to do; but I am just trying to prevent too much traffic from hitting the CI when things get merged in. We can discuss in the group meeting.

@AlexandreSinger AlexandreSinger deleted the feature-disable-ci-on-push branch July 16, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra Project Infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants