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

CI: Avoid path filtering in "required" workflows #28277

Merged
merged 2 commits into from
Aug 1, 2023

Conversation

OnkarRuikar
Copy link
Contributor

@OnkarRuikar OnkarRuikar commented Jul 31, 2023

The workflow checks only files/en-us/_redirects.txt file. It doesn't work on any other file.

It is not required to run this workflow in every PR. This PR reduces the workflows scope to only the _redirects.txt file and the workflow file.

Also, there is no point in making this workflow "required" in settings.

Edit: Had to revert the original changes due to the edge case. Now the PR focuses only on avoiding path filtering.

/cc @bsmth

@OnkarRuikar OnkarRuikar requested a review from a team as a code owner July 31, 2023 06:45
@github-actions github-actions bot added the system [PR only] Infrastructure and configuration for the project label Jul 31, 2023
Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

Thanks, @OnkarRuikar - good cleanup, leaving a +1 👍🏻

there is no point in making this workflow "required" in settings.

Why not? It seems like one of the more useful jobs to have PRs block if the author made a tooling mistake that we don't want to land on main:

https://github.com/mdn/content/actions/runs/5387620912/job/14582323137

@OnkarRuikar OnkarRuikar force-pushed the check_redirects branch 2 times, most recently from 8689843 to f5abc65 Compare July 31, 2023 09:23
@OnkarRuikar
Copy link
Contributor Author

OnkarRuikar commented Jul 31, 2023

there is no point in making this workflow "required" in settings.

Why not? It seems like one of the more useful jobs to have PRs block if the author made a tooling mistake that we don't want to land on main:

True! We do need these as required. Another (following) issue was lingering in my mind that is why the statement.

I was also thinking of the issue of these "required" workflows not running on other files and disabling the merge button. In other words we don't want to run redirects-check command for all the PRs but we do want to run the workflow as it is "required". GitHub docs suggests not to use path filter for "required" workflows.

Warning: If a workflow is skipped due to path filtering, branch filtering or a commit message, then checks associated with that workflow will remain in a "Pending" state. A pull request that requires those checks to be successful will be blocked from merging.

For this reason you should not use path or branch filtering to skip workflow runs if the workflow is required. For more information, see "Skipping workflow runs" and "Required workflows."

If, however, a job within a workflow is skipped due to a conditional, it will report its status as "Success". For more information, see "Using conditions to control job execution."

So updating these workflows using dorny/paths-filter action as requested by @Josh-Cena.

@yin1999
Copy link
Member

yin1999 commented Jul 31, 2023

Hi Onkar, in translated-content, we have occured the following case:

There is a redirection from some-old-slug to new-slug, but the contributor just created a document and the slug is some-old-slug, when we merge this PR, the redirection rule is broken. But in this case, we have merge one PR that have moved documents (by sync-translated-content). I'm not sure is there any edge case may break the redirection rule.

@OnkarRuikar OnkarRuikar changed the title CI: Fix pr-check_redirects.yml workflow file CI: Avoid path filtering in "required" workflows Aug 1, 2023
.github/workflows/pr-test.yml Outdated Show resolved Hide resolved
Co-authored-by: Brian Thomas Smith <[email protected]>
@OnkarRuikar
Copy link
Contributor Author

but the contributor just created a document and the slug is some-old-slug,

This is a very rare case but it is possible so putting the old "files/**" back.

So the the PRs scope has now reduced to only avoiding path filtering for "required" workflows. I've changed the PR title and updated the code to do just that.

Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

Thanks @OnkarRuikar - I think we're good to go

@bsmth bsmth merged commit a89079a into mdn:main Aug 1, 2023
7 of 8 checks passed
@OnkarRuikar OnkarRuikar deleted the check_redirects branch August 1, 2023 09:34
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
system [PR only] Infrastructure and configuration for the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants