Skip to content

Conversation

margaretjgu
Copy link
Member

@margaretjgu margaretjgu commented Oct 20, 2025

Update workflow file to prevent stuck/hanging pipelines for .md only prs.
as we saw here: #3041 (comment)

@prodsecmachine
Copy link

prodsecmachine commented Oct 20, 2025

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

Copy link
Member

@JoshMock JoshMock left a comment

Choose a reason for hiding this comment

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

This approach will cause the whole workflow to fail if any unit tests fail OR are cancelled. The cancellations only happen when tests are skipped because no code changed, which means all .md PRs would immediately fail.

The underlying blocker is that docs-only PRs cause unit tests to be skipped entirely (this was done to save time and compute resources), but branch rules require unit tests on all PRs and there's no easy way to change that rule to only apply sometimes. So, a better approach would be to update test and test-bun jobs to always run, but have their Unit test step check paths-filter and exit 0 if it's false, causing the test jobs to succeed without running the tests.

@margaretjgu
Copy link
Member Author

Thanks for the feedback @JoshMock! Your approach is a better solution and i've updated the workflow accordingly.

To validate my understanding, and from what I understood... jobs with false if conditions are marked as skipped, not cancelled. The cancelled state only occurs when jobs are manually stopped or overwritten by new commits. So needs.test.result == 'cancelled' would evaluate to false for skipped jobs, meaning the summary job would have succeeded for .md only PRs.

@JoshMock
Copy link
Member

Ahh good catch. I forgot "skipped" and "cancelled" were different.

Copy link
Member

@JoshMock JoshMock left a comment

Choose a reason for hiding this comment

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

I think if we just tweak these two bits, we'll be good to go.

Comment on lines +56 to 58
if: needs.paths-filter.outputs.src-only == 'true'
run: |
npm run test:unit
Copy link
Member

@JoshMock JoshMock Oct 20, 2025

Choose a reason for hiding this comment

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

Suggested change
if: needs.paths-filter.outputs.src-only == 'true'
run: |
npm run test:unit
env:
CODE_CHANGED: ${{needs.paths-filter.outputs.src-only}}
run: |
[ "$CODE_CHANGED" = "true" ] && npm run test:unit || exit 0

This is the step that branch rules require to run. An if condition will skip it (I think). Using an environment variable ensures it always runs, and will just exit early if src-only isn't true.

bun run lint
- name: Unit test
if: needs.paths-filter.outputs.src-only == 'true'
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants