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

feat(script): convert matrix scripts #318

Merged
merged 7 commits into from
Apr 4, 2022
Merged

Conversation

shortcuts
Copy link
Member

@shortcuts shortcuts commented Mar 31, 2022

🧭 What and Why

🎟 JIRA Ticket: https://algolia.atlassian.net/browse/APIC-402

Changes included:

Follow up of #170

In #313, we can see that the generated code for the PHP client is only cached for the search client, while every clients have changed.

This is due overlapping path and hash which requires more informations at the matrix level. As iterating on bash scripts is pretty hard, this PR converts our matrix generation scripts to TypeScript, and also should solve the path issue for PHP. (should because I'm not sure if the path is well scoped)

🧪 Test

  • CI :D

@netlify
Copy link

netlify bot commented Mar 31, 2022

Deploy Preview for api-clients-automation ready!

Name Link
🔨 Latest commit 4545b78
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/624ab1f9da0f8f0008a507c6
😎 Deploy Preview https://deploy-preview-318--api-clients-automation.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@algolia-bot
Copy link
Collaborator

algolia-bot commented Mar 31, 2022

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the generated/main branch.

@shortcuts shortcuts self-assigned this Mar 31, 2022
@shortcuts shortcuts requested review from millotp, a team and eunjae-lee and removed request for a team March 31, 2022 14:39
@shortcuts shortcuts marked this pull request as ready for review March 31, 2022 14:39
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 nice refacto, much cleaner and easier to read !

if: ${{ inputs.job == 'cts' || inputs.job == 'codegen' }}
uses: actions/cache@v2
with:
path: clients/algoliasearch-client-java-2
key: |
${{ env.CACHE_VERSION }}-${{
hashFiles(
'clients/algoliasearch-client-java-2/**',
'clients/algoliasearch-client-java-2/search/**',
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should also include generators and scripts, but maybe we need a better system, you can see the dependencies here

Copy link
Member Author

@shortcuts shortcuts Apr 4, 2022

Choose a reason for hiding this comment

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

I think we will need to at least re-arrange our folder structure for the CTS and Scripts in order to be able to scope those more easily, I'll put the cache to the scripts folder for now but this should be in a generation/build related folder

Copy link
Member Author

Choose a reason for hiding this comment

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

There is node_modules in scripts, mostly because of different versions in our package.json. I'll fix it in an other PR and add back the keys 😓

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, but changes to package version should also invalidate the cache

scripts/ci/createMatrix.ts Outdated Show resolved Hide resolved
scripts/ci/createMatrix.ts Outdated Show resolved Hide resolved
scripts/ci/createMatrix.ts Outdated Show resolved Hide resolved
scripts/ci/createMatrix.ts Outdated Show resolved Hide resolved
scripts/ci/createMatrix.ts Outdated Show resolved Hide resolved
scripts/ci/createMatrix.ts Outdated Show resolved Hide resolved
scripts/ci/createMatrix.ts Outdated Show resolved Hide resolved
scripts/ci/createMatrix.ts Outdated Show resolved Hide resolved
scripts/ci/createMatrix.ts Outdated Show resolved Hide resolved
@shortcuts shortcuts requested a review from millotp April 4, 2022 08:52
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.

Thanks for all the changes :)

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