Skip to content
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

Improvement: Change review_requested trigger to issue_comment trigger for full test suite #895

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from

Conversation

costigt-dev
Copy link
Collaborator

@costigt-dev costigt-dev commented Mar 6, 2024

Reason for this PR

#869

Full test suite is currently run when a review is requested which means that if several people were selected as reviewers it would trigger multiple wasteful runs of the tests.

Changes Made in this PR

This PR removes the review_requested trigger and instead replaces it with an issue_comment trigger, along with necessary conditions to ensure it only runs when pull request comments are made containing the message /run-tests

Testing Summary

The issue_comment trigger will only run if on the default branch so testing was done by merging changes into fork version of master branch and then running trigger messages on a test PR.

Risk Highlight

  • This PR includes code from another work (please detail).
  • This PR contains API-breaking changes.
  • This PR depends on work in another PR (please provide links/details).
  • This PR introduces new dependencies (please detail).
  • There are coverage gaps not covered by tests.
  • Documentation updates required in subsequent PR.

Checklist

  • Code comments added to any hard-to-understand areas, if applicable.
  • Changes generate no new warnings.
  • Updated any relevant tests, if applicable.
  • No conflicts with destination dev branch.
  • I reviewed my own code changes.
  • Initial CI/CD passing.
  • 1+ reviews given, and any review issues addressed and approved.
  • Post-review full CI/CD passing.

Future Work

@costigt-dev costigt-dev changed the title Ignore Improvement: Change review_requested trigger to issue_comment trigger for full test suite Mar 7, 2024
@costigt-dev costigt-dev marked this pull request as ready for review March 7, 2024 12:11
@costigt-dev costigt-dev requested a review from Giuseppe5 March 7, 2024 13:39
@Giuseppe5
Copy link
Collaborator

/run-tests

@costigt-dev
Copy link
Collaborator Author

costigt-dev commented Mar 8, 2024

@Giuseppe5

Structure of the workflow was tested in separate branch but ultimately the issue_comment trigger will only run if on the default branch.

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#issue_comment

@Giuseppe5
Copy link
Collaborator

Structure of the workflow was tested in separate branch but ultimately the issue_comment trigger will only run if on the default branch.

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#issue_comment

So we need to have this in master to work?

@costigt-dev
Copy link
Collaborator Author

Structure of the workflow was tested in separate branch but ultimately the issue_comment trigger will only run if on the default branch.
https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#issue_comment

So we need to have this in master to work?

yep - it's not a great design on the github actions side. Several events ONLY happen on the master branch so its a bit of a leap of faith adding them. So we'll want to scrutinize what i've added very carefully before merging.

@Giuseppe5
Copy link
Collaborator

You can try to merge this in master in your fork and see what happens there?

@costigt-dev
Copy link
Collaborator Author

You can try to merge this in master in your fork and see what happens there?

excellent idea!

@costigt-dev costigt-dev added the do not merge This should not be merged just yet label Mar 8, 2024
@costigt-dev
Copy link
Collaborator Author

You can try to merge this in master in your fork and see what happens there?

excellent idea!

Works in fork

@costigt-dev costigt-dev removed the do not merge This should not be merged just yet label Mar 11, 2024
@costigt-dev costigt-dev requested a review from nickfraser March 11, 2024 14:40
@capnramses capnramses added the infrastructure Anything related to CI/CD label Mar 11, 2024
@capnramses capnramses linked an issue Mar 11, 2024 that may be closed by this pull request
3 tasks
@costigt-dev costigt-dev added the do not merge This should not be merged just yet label Mar 14, 2024
@costigt-dev costigt-dev removed the do not merge This should not be merged just yet label Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Anything related to CI/CD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change how the automatic tests are triggered
3 participants