Skip to content

feat(ci): add cache and job skip #60

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

Merged
merged 3 commits into from
Jan 5, 2022
Merged

Conversation

shortcuts
Copy link
Member

@shortcuts shortcuts commented Jan 5, 2022

🧭 What and Why

🎟 JIRA Tickets:

Changes included:

This PR aims at improving our CI run time which is now around 8 minutes.

  • Split jobs to allow parallelization (specs and client generation/build)
  • Add cache per job (generated clients, dependencies)
  • Skip tasks based on diff (only checking with main, but should be with the target branch to improve stacked PRs)

I'll change the required checks once this PR is ready to be merged

Task skipping

  • setup: runs everytime
  • specs_*: runs when GitHub actions, scripts or spec change (e.g. only runs specs_search when specs/search changes)
  • client_*: runs when GitHub actions, scripts, language template or related spec change (e.g. only runs client_javascript_search when specs/search changes)
  • cts: runs everytime except scripts

Comparison

Previous workflow

Total duration: ~7m/8m

Current workflow

Total duration: uncached 5m 27s | cached 3m 44s

TODO

Clients are cached based on their dep file, we should find an other way

🧪 Test

CI :D

@shortcuts shortcuts requested review from millotp and a team January 5, 2022 12:48
@shortcuts shortcuts self-assigned this Jan 5, 2022
@shortcuts
Copy link
Member Author

note: I wanted to use env var to make the thing less verbose, but it seems that adding env with composite actions override some of the values

Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

Very impressive ! I did not try every combination but this looks good !

runs-on: ubuntu-20.04
needs: setup
if: ${{ always() && needs.setup.outputs.RUN_SPECS_SEARCH == 'true' }}
Copy link
Collaborator

@millotp millotp Jan 5, 2022

Choose a reason for hiding this comment

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

What does always() do ?

Copy link
Member Author

Choose a reason for hiding this comment

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

So jobs that needs a job that has been skipped is not also skipped (not sure if it's clear)

Copy link
Collaborator

Choose a reason for hiding this comment

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

for example we can skip the spec but still run the cts ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't get the && then, for me always is always true right ?

Copy link
Member Author

Choose a reason for hiding this comment

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

For example, if you change the JavaScript mustache template, it will skip the specs_* jobs since no spec changes but still run the clients_javascript* jobs, and as the clients_javascript* have changed, it will run the CTS

echo "::set-output name=JAVA_CLIENT_CHANGED::$(git diff --shortstat origin/${{ github.base_ref }}..HEAD -- clients/algoliasearch-client-java-2 | wc -l)"
echo "::set-output name=JAVA_TEMPLATE_CHANGED::$(git diff --shortstat origin/${{ github.base_ref }}..HEAD -- templates/java | wc -l)"

outputs:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very ingenious !

Copy link
Member Author

Choose a reason for hiding this comment

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

😊

@shortcuts shortcuts requested a review from millotp January 5, 2022 15:51

# js client variables
RUN_JS_CLIENT_SEARCH:
description: 'Determine if the `client_javascript_search` job should run'
value: ${{ steps.diff.outputs.GITHUB_ACTIONS_CHANGED > 0 || steps.diff.outputs.SEARCH_SPECS_CHANGED > 0 || steps.diff.outputs.SCRIPTS_CHANGED > 0 || steps.diff.outputs.JS_SEARCH_CLIENT_CHANGED > 0 || steps.diff.outputs.JS_TEMPLATE_CHANGED > 0 }}
value: ${{ steps.diff.outputs.GITHUB_ACTIONS_CHANGED > 0 || steps.diff.outputs.COMMON_SPECS_CHANGED > 0 || steps.diff.outputs.SEARCH_SPECS_CHANGED > 0 || steps.diff.outputs.SCRIPTS_CHANGED > 0 || steps.diff.outputs.JS_SEARCH_CLIENT_CHANGED > 0 || steps.diff.outputs.JS_TEMPLATE_CHANGED > 0 }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to use the RUN_SPECS_SEARCH value here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not managed to do it, it would be ideal indeed

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.

2 participants