feat(ci): use GitHub artifacts POC #469
Conversation
✅ Deploy Preview for api-clients-automation canceled.
|
✗ The generated branch has been deleted.If the PR has been merged, you can check the generated code on the |
shortcuts
force-pushed
the
chore/github-artifacts
branch
from
13bd250 to
86f4066
Compare
specs/search/paths/search/search.yml
Outdated
| summary: Search in an index THIS_IS_A_TEST. | ||
| description: Perform a search operation targeting one specific index THIS_IS_A_TEST. |
There was a problem hiding this comment.
This is only here to have a generated change to test
| @@ -0,0 +1,326 @@ | |||
| name: Restore all artifacts | |||
There was a problem hiding this comment.
This is the most annoying part of artifacts, it does not restore at the path that was stored, so we have to implicitly set it, there might be a way to either:
- Generate this file
- Make the thing less redundant
There was a problem hiding this comment.
Ok it's a bit better now but still redundant
| @@ -402,10 +381,20 @@ jobs: | |||
| - name: Check JavaScript client size | |||
| run: exit $(yarn workspace algoliasearch-client-javascript test:size | echo $?) | |||
|
|
|||
| # ----> This can be added once the PHP build for the CTS is fixed | |||
There was a problem hiding this comment.
shortcuts
changed the title
.github/actions/setup/action.yml
Outdated
| steps.diff.outputs.JS_COMMON_CHANGED > 0 }} | ||
| algoliasearch_changed=${{ steps.diff.outputs.JS_ALGOLIASEARCH_CHANGED > 0 }} | ||
| common_changed=${{ steps.diff.outputs.JS_COMMON_CHANGED > 0 }} | ||
| steps.diff.outputs.JS_UTILS_CHANGED > 0 || |
There was a problem hiding this comment.
We loose all the granularity here, we can change one template without affecting the other languages. the LANGUAGE_TEMPLATE_CHANGED should be computed inside the script for each language, or passed here but it will be verbose.
There was a problem hiding this comment.
I agree!
I have an other change locally that moves the base_changed logic in TS but I thought it was too much for this PR
.github/workflows/check.yml
Outdated
| name: client-javascript-utils-${{ matrix.client }} | ||
| path: clients/algoliasearch-client-javascript/packages/${{ matrix.client }} | ||
|
|
||
| client_gen: |
There was a problem hiding this comment.
I really think this should be split by language instead of trying to factorize, for example the Java client need to be built at the end or it won't work in some cases, and doing that logic here is very complicated.
I'm not sure this makes it easier to read either, it's very nice to remove duplication but hard with yaml
There was a problem hiding this comment.
Yeah the main goal was just to remove the language logic but I'm not a fan of it looking again
I still wonder if making the CLI allow variadic params could be easier to handle those cases, but idk how it would impact the job time
There was a problem hiding this comment.
I will remove logic this in my next PR to allow for client_java_build, like here, are you sure you want to keep that code ?
shortcuts
force-pushed
the
chore/github-artifacts
branch
from
225a62d to
8c8ac94
Compare
|
|
||
| echo "Running spec matrix: $run" | ||
| echo "Spec matrix: $(echo $matrix | jq .)" | ||
| run: yarn workspace scripts createMatrix ${{ steps.diff.outputs.ORIGIN_BRANCH }} |
There was a problem hiding this comment.
This could be in the same step as the one above
There was a problem hiding this comment.
Indeed because it does not require output variables, but (1st I did not know it would work :D) and also it's more explicit what it does like this
| /** | ||
| * List of dependencies based on the language, inherited from `./setRunVariables.ts` in a more dynamic form. | ||
| */ | ||
| const MATRIX_DEPENDENCIES = { |
There was a problem hiding this comment.
Maybe this could just hold the name of the property, like 'GITHUB_ACTIONS_CHANGED' and fetch it from DEPENDENCIES in the isBaseChanged function, as you want.
| const clientChanges = await getNbGitDiff({ | ||
| branch: baseBranch, | ||
| path: output, |
There was a problem hiding this comment.
The output is not committed so why check for it here ? the diff should be based on the same dependencies as the cache I think
There was a problem hiding this comment.
This was already here before but IIRC it's mostly for changes related to non-generated changes, that are pushed (package.json, utils packages etc.)
|
|
||
| return `${baseName}-${crypto | ||
| .createHash('sha256') | ||
| .update(`${await commonCacheKey}-${hash}`) |
There was a problem hiding this comment.
This will still get computed for each cache key, can you put it in a variable without the await ?
There was a problem hiding this comment.
wdym? commonCacheKey is only called once a createMatrix call with this
There was a problem hiding this comment.
your right this is a very weird way of doing it ahah
There was a problem hiding this comment.
I wanted to do top level await but can't manage to make it work, if you have an alternative lmk!
There was a problem hiding this comment.
I was thinking of calling it inside createMatrix function and setRunVariable function, just as a setup
There was a problem hiding this comment.
and it will set a global variable
There was a problem hiding this comment.
Ah yes out of the utils, I don't really mind I can change if you want
There was a problem hiding this comment.
it works like that don't mind
|
|
||
| return `${baseName}-${crypto | ||
| .createHash('sha256') | ||
| .update(`${await commonCacheKey}-${hash}`) |
There was a problem hiding this comment.
your right this is a very weird way of doing it ahah
algolia/api-clients-automation#469 Co-authored-by: Clément Vannicatte <[email protected]>
🧭 What and Why
🎟 JIRA Ticket: https://algolia.atlassian.net/browse/APIC-455
Changes included:
This PR adds GitHub artifact to the CI, used to pass data from a job to an other.
All the previous logic related to the cache has been removed, we now use cache as an easy way to skip jobs.
The client matrix has been grouped so we don't have to handle languages from here
🧪 Test
CI :D