-
Notifications
You must be signed in to change notification settings - Fork 18
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): re-use workflows, use composite action, cache built client. #31
Conversation
(Will update edit: NVM, it works |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job ! The CI looks really clean !
.github/actions/cache/action.yml
Outdated
path: '**/node_modules' | ||
key: ${{ runner.os }}-modules-${{ hashFiles('**/yarn.lock') }} | ||
|
||
- name: Restore JavaScript client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be cached, we want to test if this is correctly generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed it for now as I don't have a solution yet, but we should cache clients to avoid regenerating every clients when a spec change.
I'll make sure to tackle it ASAP
.github/workflows/check.yml
Outdated
- name: Restore cache | ||
uses: ./.github/actions/cache | ||
|
||
- name: Generate client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to generate the client here if this depends on client_javascript
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless cached, jobs
are running on a fresh machine, the needs
field is only here to let the Action know when to start the job
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok no problem, so the cache makes more sense now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but wasn't right on the current implementation, we should definitely have one in a near future
🧭 What and Why
🎟 JIRA Ticket: https://algolia.atlassian.net/browse/APIC-219
Changes included:
composite
actions to remove duplicate code/logicjobs
depends on other needed jobsyamllint
linter to the CI🧪 Test
CI :D