Skip to content

chore(ci): prevent commit including generated files #379

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

Merged
merged 27 commits into from
Apr 22, 2022
Merged

Conversation

eunjae-lee
Copy link
Contributor

@eunjae-lee eunjae-lee commented Apr 14, 2022

🧭 What and Why

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

Changes included:

This PR includes pre-commit hook to check if any generated file is staged. If so, it throws and the commit is canceled. The hook doesn't automatically unstage the file, but informs which files are caught.

There is some tricky case to cover, so had to write either JS/TS instead of bash script.

And I chose JS here, to run it as quickly as possible. ts-node is too slow to be run every time we run git commit.

TODO

  • check the commit message, and if it contains [skip ci], then it should really skip this test too. Otherwise, commits like this will fail.

🧪 Test

  • CI

@netlify
Copy link

netlify bot commented Apr 14, 2022

Deploy Preview for api-clients-automation canceled.

Name Link
🔨 Latest commit 32dd38e
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/6262ae12c865fd00093e12d3

@algolia-bot
Copy link
Collaborator

algolia-bot commented Apr 14, 2022

✗ The generated branch has been deleted.

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

@eunjae-lee eunjae-lee marked this pull request as ready for review April 15, 2022 12:12
@eunjae-lee eunjae-lee requested review from a team, damcou and shortcuts and removed request for a team April 15, 2022 12:12
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.

This is perfect thank you ! it will solve all of our process issues 🙏

@millotp
Copy link
Collaborator

millotp commented Apr 19, 2022

It can be hard when we have a lot of files to remove the correct files, like when we rename a folder or something, can you add a little script that uses the same logic just to unstage generated files pls ?

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.

love it!! GG

@shortcuts
Copy link
Member

Maybe now that we have a defined list of files generated and not generated we can improve the delete models function and delete all the generated code before generation

(I've made an update on the codegen removing on #386)

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.

Such a nice improvement !

'!clients/**/.openapi-generator-ignore',

// Java
'!clients/algoliasearch-client-java-2/algoliasearch-core/com/algolia/exceptions/*',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you update those path pls ? They now use algoliasearch-core/src/com/algolia...

// Java
'!clients/algoliasearch-client-java-2/algoliasearch-core/com/algolia/exceptions/*',
'!clients/algoliasearch-client-java-2/algoliasearch-core/com/algolia/utils/*',
'clients/algoliasearch-client-java-2/algoliasearch-core/com/algolia/utils/echo/EchoResponse.java',
Copy link
Collaborator

@millotp millotp Apr 22, 2022

Choose a reason for hiding this comment

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

The echo response is a bit more complicated now, its:

Suggested change
'clients/algoliasearch-client-java-2/algoliasearch-core/com/algolia/utils/echo/EchoResponse.java',
'clients/algoliasearch-client-java-2/algoliasearch-core/src/com/algolia/utils/echo/EchoResponse*.java',
'!clients/algoliasearch-client-java-2/algoliasearch-core/src/com/algolia/utils/echo/EchoResponseInterface.java',

const execa = require('execa');
const micromatch = require('micromatch');

const GENERATED_FILE_PATTERNS = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

This list is meant to be updated whenever we create a file by hand, maybe it would be nice to have it easily accessible in the config folder, wdyt ?

Copy link
Member

Choose a reason for hiding this comment

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

^!!

Copy link
Collaborator

Choose a reason for hiding this comment

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

(and maybe documented somewhere on how to update this list)

Copy link
Contributor Author

@eunjae-lee eunjae-lee Apr 22, 2022

Choose a reason for hiding this comment

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

I created config/generation.config.js. What do you think?

I created a json first, but that way we cannot add any comment. This config particularly will require us to write some contexts as comment. So I made it a js file. Let me know what you think. 73528a0

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's perfect like that !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@millotp great! and I applied the changes to the patterns in that js file.

const negativeMatchers = [];

patterns.forEach((pattern) => {
if (pattern.startsWith('!')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure this simplification of splitting negative and positive will work for every case, for example if we have a sequence of [positive, negative, positive, negative], it will loose the pattern, but I don't have a better solution 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'm not too sure either. But I couldn't think of any edge case so far. Once we face it in the future, it will reveal itself and still won't ruin anything. Then we can adjust the logic to cover the case.

continue;
}

console.log(
Copy link
Collaborator

Choose a reason for hiding this comment

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

really nice output 😃

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.

Really nice! Good to go after Pierre's suggestions

@@ -0,0 +1,24 @@
/* eslint-disable import/no-commonjs */
Copy link
Member

Choose a reason for hiding this comment

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

Test can still be in TS 😇

Copy link
Member

Choose a reason for hiding this comment

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

(joking)

const execa = require('execa');
const micromatch = require('micromatch');

const GENERATED_FILE_PATTERNS = [
Copy link
Member

Choose a reason for hiding this comment

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

^!!

@shortcuts
Copy link
Member

@eunjae-lee can you bump the cache key? idk why it works here but not on main

@eunjae-lee eunjae-lee requested a review from shortcuts April 22, 2022 14:00
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.

That's nice!!!! GG

@@ -0,0 +1,30 @@
// eslint-disable-next-line import/no-commonjs
module.exports = {
Copy link
Member

Choose a reason for hiding this comment

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

We can maybe add a description of what this file does exactly

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.

Actually the doc is enough for the file so feel free to ignore

@eunjae-lee eunjae-lee merged commit 9fbba0e into main Apr 22, 2022
@eunjae-lee eunjae-lee deleted the chore/githook branch April 22, 2022 14:23
shortcuts pushed a commit that referenced this pull request Apr 22, 2022
* chore(ci): add pre-commit hook to unstage generated codes

* chore: test commit

* chore: implement pre-commit script

* chore: fix lint error

* chore: fix lint error

* chore: call js file directly

* chore: update .cache_version

* Update scripts/ci/husky/pre-commit.js

Co-authored-by: Pierre Millot <[email protected]>

* Update scripts/ci/husky/pre-commit.js

Co-authored-by: Pierre Millot <[email protected]>

* chore: use postinstall instead of prepare

* chore: better error message

* chore: fix patterns

* chore: unstage files automatically and exit with 0

* chore: change loop

* chore: skip deleted files

* chore: split config to a separate file

* chore: update docs

* chore: update cache version

Co-authored-by: Pierre Millot <[email protected]>
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.

4 participants