Fix: Build Workflow Runs All Jobs Twice

Matthew-Grayson commented 3 months ago

🗣 Description

EDIT: Add conditional to build jobs so they do not run a second time for internal PRs. ~~Remove on: pull_request logic from build.yml.~~

💭 Motivation and context

This logic causes build jobs to run twice on each pull request. This can cause rate limit issues when GitHub Actions is queuing jobs.

The on: push logic by itself runs jobs when:

A contributor pushes to a branch
A contributor creates a pull request
A codeowner merges a pull request

The on: pull_request logic runs jobs when:

A contributor creates a pull request
A contributor pushes to a branch connected to a PR

Closes #174

🧪 Testing

📷 Screenshots (if appropriate)

✅ Pre-approval checklist

[x] This PR has an informative and human-readable title.
[x] Changes are limited to a single goal - eschew scope creep!
[x] All future TODOs are captured in issues, which are referenced in code comments.
[x] All relevant type-of-change labels have been added.
[x] I have read the CONTRIBUTING document.
[x] These code changes follow cisagov code standards.
[x] All relevant repo and/or project documentation has been updated to reflect the changes in this PR.
[x] Tests have been added and/or modified to cover the changes in this PR.
[x] All new and existing tests pass.

✅ Pre-merge checklist

[ ] Revert dependencies to default branches.
[ ] Finalize version.

✅ Post-merge checklist

[ ] Create a release.

mcdonnnj commented 3 months ago

Per the documentation for push and pull_request events, the reason you see two runs is because one is for the pull request and the other is for the latest commit for the branch. If you wait for the push Actions to complete and then open a pull request you will see that the only new Actions work generated is for the pull_request event. I'm not sure how the rest of my team feels but I see no reason to change this configuration.

jsf9k commented 3 months ago

Per the documentation for push and pull_request events, the reason you see two runs is because one is for the pull request and the other is for the latest commit for the branch. If you wait for the push Actions to complete and then open a pull request you will see that the only new Actions work generated is for the pull_request event. I'm not sure how the rest of my team feels but I see no reason to change this configuration.

See also here.

Matthew-Grayson commented 3 months ago

@mcdonnnj @jsf9k Thank you for your attention on this. The motivation is to clean up the number of checks in GitHub Actions. These add noise and bog down the queue when many PRs are submitted at once (say by dependabot).

I pushed changes based on your feedback that prevent duplicate jobs, but still run jobs on PRs from a fork. But, I understand if you feel this change is an unnecessary risk to functionality. Please see https://github.com/cisagov/skeleton-generic/pull/175/commits/17af8a8bc2dbd2f9b3f7fc9d6a2ae89755901f92.

jsf9k commented 3 months ago

I pushed changes based on your feedback that prevent duplicate jobs, but still run jobs on PRs from a fork. But, I understand if you feel this change is an unnecessary risk to functionality. Please see 17af8a8.

I'm not averse to a change like this to reduce the contention for GH Actions runners, provided that is satisfies the conditions I enumerated in #174. That said, this current implementation is pretty ugly. The conditional would have to be added manually to every single job in every single workflow. Can we do something cleaner and less invasive?

mcdonnnj commented 3 months ago

I pushed changes based on your feedback that prevent duplicate jobs, but still run jobs on PRs from a fork. But, I understand if you feel this change is an unnecessary risk to functionality. Please see 17af8a8.

I'm not averse to a change like this to reduce the contention for GH Actions runners, provided that is satisfies the conditions I enumerated in #174. That said, this current implementation is pretty ugly. The conditional would have to be added manually to every single job in every single workflow. Can we do something cleaner and less invasive?

@jsf9k I think the idea is that if the diagnostics job is gated then other jobs, which rely on it, will not be able to start. I'm still of the opinion that if a downstream project wants to modify this that is their prerogative but the default is fine as it is.

Matthew-Grayson commented 3 months ago

@jsf9k I think the idea is that if the diagnostics job is gated then other jobs, which rely on it, will not be able to start. I'm still of the opinion that if a downstream project wants to modify this that is their prerogative but the default is fine as it is.

Right. Understood.

mcdonnnj commented 2 months ago

Per conversations here, in #174, and within our team we will not be incorporating this change into the skeleton.

cisagov / skeleton-generic