-
Notifications
You must be signed in to change notification settings - Fork 21
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
Changes from 14 commits
3e75735
227edcc
9167fc9
b9d1057
ff0b1b8
054c031
3fe4723
40cbd5e
c5d7781
fbfba2b
c8c3c6e
0b07721
4dc5a01
202169c
6d34e74
62c75fe
541a167
a3d66a6
fc73866
958ff64
d5fa011
958c1fa
613fca9
73528a0
b7e752e
bb700fd
32dd38e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
#!/bin/sh | ||
. "$(dirname "$0")/_/husky.sh" | ||
|
||
./scripts/ci/husky/pre-commit.js |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
/* eslint-disable import/no-commonjs */ | ||
// eslint-disable-next-line @typescript-eslint/no-var-requires | ||
const { createMemoizedMicromatchMatcher } = require('../pre-commit'); | ||
|
||
describe('createMemoizedMicromatchMatcher', () => { | ||
it('matches correctly', () => { | ||
const matcher = createMemoizedMicromatchMatcher([ | ||
'clients/**/*', | ||
'!clients/README.md', | ||
]); | ||
|
||
expect(matcher('clients/README.md')).toEqual(false); | ||
expect(matcher('clients/CONTRIBUTING.md')).toEqual(true); | ||
}); | ||
|
||
it('prioritizes the exact match when two patterns conflict', () => { | ||
const matcher = createMemoizedMicromatchMatcher([ | ||
'!lib/Configuration/*', | ||
'lib/Configuration/Configuration.php', | ||
]); | ||
eunjae-lee marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
expect(matcher('lib/Configuration/Configuration.php')).toEqual(true); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,96 @@ | ||||||||
#!/usr/bin/env node | ||||||||
eunjae-lee marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
/* eslint-disable no-process-exit */ | ||||||||
/* eslint-disable no-console */ | ||||||||
/* eslint-disable import/no-commonjs */ | ||||||||
/* eslint-disable @typescript-eslint/no-var-requires */ | ||||||||
eunjae-lee marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
const execa = require('execa'); | ||||||||
const micromatch = require('micromatch'); | ||||||||
|
||||||||
const GENERATED_FILE_PATTERNS = [ | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ^!! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (and maybe documented somewhere on how to update this list) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I created 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's perfect like that ! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||||
// Ignore the roots and go down the tree by negating hand written files | ||||||||
'clients/**/*', | ||||||||
shortcuts marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
'!clients/README.md', | ||||||||
'!clients/**/.openapi-generator-ignore', | ||||||||
|
||||||||
// Java | ||||||||
'!clients/algoliasearch-client-java-2/algoliasearch-core/com/algolia/exceptions/*', | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you update those path pls ? They now use |
||||||||
'!clients/algoliasearch-client-java-2/algoliasearch-core/com/algolia/utils/*', | ||||||||
'clients/algoliasearch-client-java-2/algoliasearch-core/com/algolia/utils/echo/EchoResponse.java', | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The echo response is a bit more complicated now, its:
Suggested change
|
||||||||
|
||||||||
// JavaScript | ||||||||
'!clients/algoliasearch-client-javascript/*', | ||||||||
'!clients/algoliasearch-client-javascript/.github/**/*', | ||||||||
'!clients/algoliasearch-client-javascript/.yarn/**/*', | ||||||||
'!clients/algoliasearch-client-javascript/scripts/**/*', | ||||||||
'!clients/algoliasearch-client-javascript/packages/algoliasearch/**/*', | ||||||||
'!clients/algoliasearch-client-javascript/packages/requester-*/**/*', | ||||||||
'!clients/algoliasearch-client-javascript/packages/client-common/**/*', | ||||||||
|
||||||||
// PHP | ||||||||
'!clients/algoliasearch-client-php/lib/Configuration/*', | ||||||||
'clients/algoliasearch-client-php/lib/*.php', | ||||||||
'clients/algoliasearch-client-php/lib/Api/*', | ||||||||
'clients/algoliasearch-client-php/lib/Configuration/Configuration.php', | ||||||||
]; | ||||||||
|
||||||||
const run = async (command, { cwd } = {}) => { | ||||||||
return ( | ||||||||
(await execa.command(command, { shell: 'bash', all: true, cwd })).all ?? '' | ||||||||
); | ||||||||
}; | ||||||||
eunjae-lee marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
||||||||
function createMemoizedMicromatchMatcher(patterns = []) { | ||||||||
eunjae-lee marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
const exactMatchers = []; | ||||||||
const positiveMatchers = []; | ||||||||
const negativeMatchers = []; | ||||||||
|
||||||||
patterns.forEach((pattern) => { | ||||||||
if (pattern.startsWith('!')) { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||||
// Patterns starting with `!` are negated | ||||||||
negativeMatchers.push(micromatch.matcher(pattern.slice(1))); | ||||||||
} else if (!pattern.includes('*')) { | ||||||||
exactMatchers.push(micromatch.matcher(pattern)); | ||||||||
} else { | ||||||||
positiveMatchers.push(micromatch.matcher(pattern)); | ||||||||
} | ||||||||
}); | ||||||||
|
||||||||
return function matcher(str) { | ||||||||
if (exactMatchers.some((match) => match(str))) { | ||||||||
return true; | ||||||||
} | ||||||||
|
||||||||
// As `some` returns false on empty array, test will always fail if we only | ||||||||
// provide `negativeMatchers`. We fallback to `true` is it's the case. | ||||||||
const hasPositiveMatchers = | ||||||||
Boolean(positiveMatchers.length === 0 && negativeMatchers.length) || | ||||||||
positiveMatchers.some((match) => match(str)); | ||||||||
|
||||||||
return hasPositiveMatchers && !negativeMatchers.some((match) => match(str)); | ||||||||
}; | ||||||||
} | ||||||||
|
||||||||
async function preCommit() { | ||||||||
const stagedFiles = (await run(`git diff --name-only --cached`)).split('\n'); | ||||||||
const matcher = createMemoizedMicromatchMatcher(GENERATED_FILE_PATTERNS); | ||||||||
const generatedFiles = stagedFiles.filter((file) => matcher(file)); | ||||||||
eunjae-lee marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
||||||||
if (generatedFiles.length > 0) { | ||||||||
console.log( | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. really nice output 😃 |
||||||||
'\x1b[41m%s\x1b[0m', | ||||||||
'[ERROR]', | ||||||||
`You cannot include generated files in the commit. Please unstage the following:\n\n${generatedFiles | ||||||||
.map((file) => ` - ${file}`) | ||||||||
.join('\n')}` | ||||||||
); | ||||||||
process.exit(1); | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
if (require.main === module) { | ||||||||
preCommit(); | ||||||||
} | ||||||||
|
||||||||
module.exports = { | ||||||||
createMemoizedMicromatchMatcher, | ||||||||
}; |
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.
Test can still be in TS 😇
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.
(joking)