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

chore(ci): generate the CTS on the CI #489

Merged
merged 8 commits into from
May 11, 2022
Merged

chore(ci): generate the CTS on the CI #489

merged 8 commits into from
May 11, 2022

Conversation

millotp
Copy link
Collaborator

@millotp millotp commented May 10, 2022

🧭 What and Why

After this PR, all the generated code everywhere will be pushed by the CI, no need to gen a single file locally.

Changes included:

  • Delete tmp zips
  • Push all changes to generated branch
  • Add CTS outputs to the list of files not to push

🧪 Test

CI

@millotp millotp self-assigned this May 10, 2022
@netlify
Copy link

netlify bot commented May 10, 2022

Deploy Preview for api-clients-automation canceled.

Name Link
🔨 Latest commit 3bc6873
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/627b6adbd406e600081a0b77

@algolia-bot
Copy link
Collaborator

algolia-bot commented May 10, 2022

✗ The generated branch has been deleted.

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

@millotp millotp force-pushed the chore/gen-cts-ci branch 2 times, most recently from 1b9f572 to af092f4 Compare May 10, 2022 15:19
@millotp millotp force-pushed the chore/gen-cts-ci branch from af092f4 to 0b427fd Compare May 10, 2022 15:37
);
await run(`git add ${FOLDERS_TO_CHECK}`);
console.log(`Pushing code to generated branch: '${branchToPush}'`);
await run(`git add .`);
Copy link
Member

Choose a reason for hiding this comment

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

We need to make sure not to push deleted files for the tests, as we delete the folder before

Copy link
Member

Choose a reason for hiding this comment

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

Ah but yeah I forgot, the CI does not push deleted files hehehehe

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure about this, we will see with more test I guess, anyway the CTS is pushed correctly for now ! d33031a

@millotp millotp marked this pull request as ready for review May 10, 2022 17:13
@millotp millotp requested review from a team, damcou and shortcuts and removed request for a team May 10, 2022 17:13
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

nice :)

.github/workflows/check.yml Outdated Show resolved Hide resolved
config/generation.config.js Outdated Show resolved Hide resolved
@@ -225,16 +225,13 @@ jobs:
- name: Generate CTS
Copy link
Member

Choose a reason for hiding this comment

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

Might be the right time to add the cache step for the CTS too :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The issue is that we only want to keep the CTS files for the current client being generated, I don't have that granularity with cache, you think we should pass the list of files to restore or something ?

@@ -17,7 +17,7 @@ module.exports = {
'!clients/algoliasearch-client-java-2/algoliasearch-core/src/main/java/com/algolia/exceptions/**',
'!clients/algoliasearch-client-java-2/algoliasearch-core/src/main/java/com/algolia/utils/**',

'tests/output/java/src/test/java/com/algolia/methods/**',
'tests/output/java/src/test/java/com/algolia/methods/**', // this could be added automatically by the script, but with overhead
Copy link
Member

Choose a reason for hiding this comment

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

For this file we should actually leverage the functions from scripts/config.ts or directly import fields from config/clients.config.json but it's not a big deal here (unless you want to do it)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I will do it in another PR

Copy link
Member

Choose a reason for hiding this comment

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

IMO It's not urgent to do it rn

'tests/output/java/src/test/java/com/algolia/methods/**',
'tests/output/java/src/test/java/com/algolia/client/**',
'tests/output/javascript/src/methods/**',
'tests/output/java/src/test/client/**',
Copy link
Member

Choose a reason for hiding this comment

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

javascript

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

damnit

Copy link
Member

Choose a reason for hiding this comment

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

ahaha

Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

GG!

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