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

Add path filter #857

Merged
merged 4 commits into from
Jul 27, 2022
Merged

Add path filter #857

merged 4 commits into from
Jul 27, 2022

Conversation

Etesam913
Copy link
Contributor

@Etesam913 Etesam913 commented Jul 21, 2022

Overview

  • Adds path filtering on the job level to allow for jobs to be skipped if certain files are not changed.

github-actions-architecture

The base branch is looked at(in this case add-tests-to-tips-example) to determine what files are changed.

As commit 62faad8 updates a file in the static_react_task folder, the static_react_task test gets ran and the others don't.
tests-being-ran

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 21, 2022
@Etesam913 Etesam913 changed the base branch from main to add-tests-to-tips-example July 21, 2022 21:49
@codecov-commenter
Copy link

Codecov Report

Merging #857 (62faad8) into add-tests-to-tips-example (b2126b8) will not change coverage.
The diff coverage is n/a.

@@                    Coverage Diff                     @@
##           add-tests-to-tips-example     #857   +/-   ##
==========================================================
  Coverage                      64.61%   64.61%           
==========================================================
  Files                            107      107           
  Lines                           9281     9281           
==========================================================
  Hits                            5997     5997           
  Misses                          3284     3284           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2126b8...62faad8. Read the comment docs.

Comment on lines +32 to +53
static_react_task:
- 'examples/static_react_task/**'
static_react_task_with_tips:
- 'examples/static_react_task_with_tips/**'
mnist:
- 'examples/remote_procedure/mnist/**'
template:
- 'examples/remote_procedure/template/**'
toxicity_detection:
- 'examples/remote_procedure/toxicity_detection/**'
abstractions:
- 'mephisto/abstractions/**'
data_model:
- 'mephisto/data_model/**'
operations:
- 'mephisto/operations/**'
tools:
- 'mephisto/tools/**'
mephisto-task:
- 'packages/mephisto-task/src/**'
mephisto-worker-addons:
- 'packages/mephisto-worker-addons/src/**'
Copy link
Contributor Author

@Etesam913 Etesam913 Jul 21, 2022

Choose a reason for hiding this comment

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

These are the folders that I thought can potentially break a task, so that is why they trigger the tasks

static-react-task:
needs: changes
if: ${{ (needs.changes.outputs.static_react_task == 'true') || (needs.changes.outputs.mephisto-task == 'true') || (needs.changes.outputs.abstractions == 'true') || (needs.changes.outputs.data_model == 'true') || (needs.changes.outputs.operations == 'true') || (needs.changes.outputs.tools == 'true')}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If any of the folders in this statement are modified (== 'true'), then run the test. Otherwise, skip it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done for every job in the action. Some of the predicates in the if are changed depending of the job.

For example, static_react_task_with_tips is also looking in the mephisto-worker-addons directory as it has Tips and Feedback in its core_components.jsx.

- uses: actions/checkout@v3
with:
fetch-depth: 2
- uses: dorny/paths-filter@v2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This GitHub action is used to allow for path filtering on the job level.

GitHub actions natively supports path filtering on the action level. This is too broad for this use-case as it would look something like examples/**.

This would mean that all the tests would run every time a file is changed in static_react_task.

This solution allows only for the static_react_task test to run when a file is changed in static_react_task

Copy link
Contributor

@JackUrb JackUrb left a comment

Choose a reason for hiding this comment

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

This is awesome. Filtering will definitely help overall!

One small change I'd make though, we'd expect that changes to the underlying architecture that could break mnist and toxicity_detection (our slowest tests here) would also break template, so we can drop the

(needs.changes.outputs.abstractions == 'true') || (needs.changes.outputs.data_model == 'true') || (needs.changes.outputs.operations == 'true') || (needs.changes.outputs.tools == 'true')

triggers from these cases. Otherwise this is a solid DevX improvement!

@Etesam913 Etesam913 requested a review from pringshia July 26, 2022 14:48
@Etesam913 Etesam913 merged commit ac6924e into add-tests-to-tips-example Jul 27, 2022
Sprint #14 - Pensive Cicada (Aug 1) automation moved this from In progress to Done Jul 27, 2022
This was referenced Jul 27, 2022
@pringshia pringshia deleted the add-path-filter branch July 29, 2022 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants