Skip to content
New issue

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

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

Already on GitHub? # to your account

Enable tests for PRs that don't merge into main or develop #765

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

jwallwork23
Copy link
Contributor

@jwallwork23 jwallwork23 commented Jan 2, 2025

Closes #764

I have a PR (#763) that makes edits to a branch other than main or develop. To run the tests, I needed to modify the GitHub Actions workflow. Is this intended?

Closes #769

I recommend we use this approach described here i.e.,:

Workflows that would otherwise be triggered using on: push or on: pull_request won't be triggered if you add any of the following strings to the commit message in a push, or the HEAD commit of a pull request:

  • [skip ci]
  • [ci skip]
  • [no ci]
  • [skip actions]
  • [actions skip]

I think we should avoid editing .github/workflows/test_suite.yml.

There is another popular approach using something like if: github.event.pull_request.draft == false in the workflow, but this comes with many problems. See here for more info.

@jwallwork23 jwallwork23 self-assigned this Jan 2, 2025
@jwallwork23 jwallwork23 added the ICCS Tasks or reviews for the ICCS team label Jan 2, 2025
@@ -4,7 +4,6 @@ on:
push:
branches: [ main, develop ]
pull_request:
branches: [ main, develop ]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TomMelt to look into draft PRs. If not, add a comment here for how to ignore certain branches.

@TomMelt TomMelt force-pushed the issue764_testing-not-main-or-develop branch from 6a91857 to af56639 Compare January 7, 2025 11:22
Copy link

@AdelekeBankole AdelekeBankole left a comment

Choose a reason for hiding this comment

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

@TomMelt, thank you. This looks good to me at least from what I picked up from yesterday's meeting.

@TomMelt TomMelt force-pushed the issue764_testing-not-main-or-develop branch from ac9d672 to f8b5d84 Compare January 7, 2025 11:33
See comments in `.github/workflows/test_suite.yml` for more info.
@TomMelt TomMelt force-pushed the issue764_testing-not-main-or-develop branch from f8b5d84 to 42cac55 Compare January 7, 2025 11:39
@TomMelt
Copy link
Contributor

TomMelt commented Jan 7, 2025

@timspainNERSC , I just raised #769 because I realised I could merge without the tests running at all. We should be able to fix this next meeting.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
ICCS Tasks or reviews for the ICCS team testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add branch protection to develop Tests don't run on PRs that don't merge into main or develop
3 participants