-
Notifications
You must be signed in to change notification settings - Fork 17
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: cache custom generators #311
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 |
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 idea ! It could be applied to everything ahah
return cache; | ||
} | ||
|
||
for (const fileToCache of filesToCache) { |
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 think this would be more elegant with map
and join('-')
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.
But less readable :( I'd rather keep this one
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.
by elegant I meant readable, as you prefer
Yep definitely, but already a good starting point :D |
{ | ||
job: 'custom generators', | ||
folder: toAbsolutePath('generators/'), | ||
generatedFiles: ['build'], |
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.
The issue with only checking if the generatedFiles
exists is that they are not just files, build
is a folder and if you remove something in it, the cache skip but the gen won't work.
One way to solve it would be to enforce real files in the generatedFiles
list, no folder, for example it would be build/libs/algolia-java-openapi-generator-1.0.0.jar
.
But then it would make the cache harder to use, if we really want all the files under build for the cache to be valid, then we can also store a hash of the generated files and compare that
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'll let you choose what to do here
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 went with the naive solution because I wanted to have it merged before the demo, but storing hash as we do on the CI might be the more precise solution.
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.
fun fact, I had the local changes during the demo but built #313 before so it invalidated the cache and added like 5s to the demo 🤷🏼
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.
Yeah on the CI they store a zip of the generated files it's even more hardcore
{ | ||
job: 'custom generators', | ||
folder: toAbsolutePath('generators/'), | ||
generatedFiles: ['build'], |
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'll let you choose what to do here
🧭 What and Why
🎟 JIRA Ticket: -
Changes included:
This PR extracts the caching logic of the build specs step to use it when building custom generators.
🧪 Test
CI :D