feat: Support an Impacts All PATH Filter #39
Conversation
|
/trunk merge |
|
✨ Submitted to Merge by Nikhil Birmiwal (@nikhilbirmiwal). It will be added to the graph once all branch protection rules pass and there are no merge conflicts with the target branch. (details) |
| @@ -10,6 +10,8 @@ jobs: | |||
| steps: | |||
| - name: Checkout | |||
| uses: actions/checkout@v3 | |||
| with: | |||
There was a problem hiding this comment.
Clone all. git cloning has been moved to the prerequisites step.
There was a problem hiding this comment.
Looks great! Nice job. I didn't approve yet since you have the PR queued and I wanted your thoughts on the comment about moving computing the impacted targets (the steps with if: ${{ steps.prerequisites.outputs.impacts_all_detected == 'false' }}) into its own workflow. When ready (whether you choose to do it or not) just request a rereview
|
|
||
| - name: Install Bazel in PATH | ||
| if: ${{ steps.prerequisites.outputs.requires_default_bazel_installation == 'true' }} |
There was a problem hiding this comment.
I feel like everything with if: ${{steps.prerequisites.outputs.impacts_all_detected == 'false' }} should be put into its own workflow and only run on that if, so that:
- Clear separation of logic
- You don't have to repeat that if everywhere
There was a problem hiding this comment.
I pushed a few changes to cleanup the workflow:
-- always install jq, regardless of impacts_all_detected
-- set requires_default_bazel_installation to false if impacts_all_detected==true.
I think this cleans up the workflow, PTAL.
| @@ -31,6 +35,6 @@ jobs: | |||
| shell: bash | |||
| run: | | |||
| target_count=`cat ${{ steps.compute.outputs.impacted_targets_out }} | wc -l` | |||
| if [[ $target_count -ne 2 ]]; then | |||
| if [[ $target_count -ne 0 ]]; then | |||
There was a problem hiding this comment.
We moved sha computation outside of the compute_impacted_targets script, and expect it as an input into the script. As opposed to running tests with a stable test branch and a moving main, we now run tests with a stable test branch and a stable main, such that we can pin the shas. In that case, we would expect no impacted targets to be computed.
|
Thanks -- FWIW, the PR is queued against an ephemeral trunk-io/merge-action repo, used for testing purposes only. |
prerequisites.ALL.Tested using
netkos/actagainst a temporary merge graph in staging: https://www.loom.com/share/Library-or-Loom-14-September-2023-4edd8ed7f45240718f4180a3543585dcCloses https://linear.app/trunk/issue/TRUNK-8676/support-wildcard-impacted-target