brevitas icon indicating copy to clipboard operation
brevitas copied to clipboard

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

Open costigt-dev opened this issue 1 year ago • 7 comments

Reason for this PR

https://github.com/Xilinx/brevitas/issues/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

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

Future Work

costigt-dev avatar Mar 06 '24 11:03 costigt-dev

/run-tests

Giuseppe5 avatar Mar 08 '24 09:03 Giuseppe5

@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

costigt-dev avatar Mar 08 '24 09:03 costigt-dev

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?

Giuseppe5 avatar Mar 08 '24 09:03 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

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.

costigt-dev avatar Mar 08 '24 10:03 costigt-dev

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

Giuseppe5 avatar Mar 08 '24 10:03 Giuseppe5

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

excellent idea!

costigt-dev avatar Mar 08 '24 10:03 costigt-dev

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

excellent idea!

Works in fork

costigt-dev avatar Mar 08 '24 12:03 costigt-dev