-
Notifications
You must be signed in to change notification settings - Fork 21
fix(ci): make generation comment clearer #258
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 all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
name: Codegen | ||
|
||
on: | ||
pull_request: | ||
types: [opened, synchronize, closed] | ||
|
||
jobs: | ||
notification: | ||
runs-on: ubuntu-20.04 | ||
timeout-minutes: 10 | ||
if: (github.event.action == 'opened' || github.event.action == 'synchronize') && github.event.number | ||
steps: | ||
- uses: actions/checkout@v2 | ||
with: | ||
fetch-depth: 0 | ||
shortcuts marked this conversation as resolved.
Show resolved
Hide resolved
|
||
ref: ${{ github.event.pull_request.head.ref }} | ||
|
||
- name: Setup | ||
uses: ./.github/actions/setup | ||
with: | ||
type: minimal | ||
|
||
- name: Add notification comment | ||
run: yarn workspace scripts upsertGenerationComment notification | ||
env: | ||
GITHUB_TOKEN: ${{ secrets.TOKEN_GENERATE_BOT }} | ||
PR_NUMBER: ${{ github.event.number }} | ||
|
||
cleanup: | ||
runs-on: ubuntu-20.04 | ||
timeout-minutes: 10 | ||
if: github.event.action == 'closed' | ||
steps: | ||
- uses: actions/checkout@v2 | ||
with: | ||
fetch-depth: 0 | ||
ref: main | ||
|
||
- name: Setup | ||
uses: ./.github/actions/setup | ||
with: | ||
type: minimal | ||
|
||
- name: Clean previously generated branch | ||
shortcuts marked this conversation as resolved.
Show resolved
Hide resolved
|
||
run: yarn workspace scripts cleanGeneratedBranch ${{ github.head_ref }} | ||
|
||
- name: Add cleanup comment | ||
run: yarn workspace scripts upsertGenerationComment cleanup | ||
env: | ||
GITHUB_TOKEN: ${{ secrets.TOKEN_GENERATE_BOT }} | ||
PR_NUMBER: ${{ github.event.number }} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
import { cleanGeneratedBranch } from '../cleanGeneratedBranch'; | ||
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. Once again you should not import files with global code, you can wrap the global code in an if 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. Ah yes I just did in the last commit actually to follow what Eunjae did on the other tests 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. cool ! That way you can also call directly the function instead of using |
||
import { pushGeneratedCode } from '../pushGeneratedCode'; | ||
import commentText from '../text'; | ||
import { | ||
getCommentBody, | ||
upsertGenerationComment, | ||
} from '../upsertGenerationComment'; | ||
|
||
describe('codegen', () => { | ||
describe('cleanGeneratedBranch', () => { | ||
it('throws without parameters', async () => { | ||
await expect( | ||
// @ts-expect-error a parameter is required | ||
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. You can pass 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. It wouldn't be accurate as we actually don't accept |
||
cleanGeneratedBranch() | ||
).rejects.toThrow( | ||
'The base branch should be passed as a cli parameter of the `cleanGeneratedBranch` script.' | ||
); | ||
}); | ||
}); | ||
|
||
describe('pushGeneratedCode', () => { | ||
it('throws without GITHUB_TOKEN environment variable', async () => { | ||
await expect(pushGeneratedCode()).rejects.toThrow( | ||
'Environment variable `GITHUB_TOKEN` does not exist.' | ||
); | ||
}); | ||
}); | ||
|
||
describe('upsertGenerationComment', () => { | ||
it('throws without parameter', async () => { | ||
await expect( | ||
// @ts-expect-error a parameter is required | ||
upsertGenerationComment() | ||
).rejects.toThrow( | ||
'`upsertGenerationComment` requires a `trigger` parameter (`codegen` | `notification`).' | ||
); | ||
}); | ||
|
||
it('throws without PR_NUMBER environment variable', async () => { | ||
process.env.GITHUB_TOKEN = 'foo'; | ||
|
||
await expect(upsertGenerationComment('codegen')).rejects.toThrow( | ||
'`upsertGenerationComment` requires a `PR_NUMBER` environment variable.' | ||
); | ||
}); | ||
}); | ||
|
||
describe('getCommentBody', () => { | ||
it('returns the right comment for a `notification` trigger', async () => { | ||
expect(await getCommentBody('notification')).toMatchInlineSnapshot(` | ||
"🔨 The codegen job will run at the end of the CI. | ||
|
||
_Make sure your last commit does not contains generated code, it will be automatically pushed by our CI._" | ||
`); | ||
}); | ||
|
||
it('returns the right comment for a `noGen` trigger', async () => { | ||
expect(await getCommentBody('noGen')).toMatchInlineSnapshot(` | ||
"✗ No code generated. | ||
|
||
_If you believe this is an issue on our side, please [open an issue](https://github.com/algolia/api-clients-automation/issues/new?template=Bug_report.md)._" | ||
`); | ||
}); | ||
|
||
it('returns the right comment for a `cleanup` trigger', async () => { | ||
expect(await getCommentBody('cleanup')).toMatchInlineSnapshot(` | ||
"✗ The generated branch has been deleted. | ||
|
||
If the PR has been merged, you can check the generated code on the [\`generated/main\` branch](https://github.com/algolia/api-clients-automation/tree/generated/main)." | ||
`); | ||
}); | ||
|
||
describe('text', () => { | ||
it('creates a comment body for the parameters', () => { | ||
expect( | ||
commentText.codegen.body( | ||
'myBranch', | ||
'myCommit', | ||
42, | ||
'theGeneratedCommit' | ||
) | ||
).toMatchInlineSnapshot(` | ||
"🔨 Triggered by commit [myCommit](https://github.com/algolia/api-clients-automation/pull/42/commits/myCommit). | ||
🔍 Browse the generated code on branch [myBranch](https://github.com/algolia/api-clients-automation/tree/myBranch): [theGeneratedCommit](https://github.com/algolia/api-clients-automation/commit/theGeneratedCommit)." | ||
`); | ||
}); | ||
}); | ||
}); | ||
}); |
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.
Because this file is quite different with
minimal
, is it worth it to create another file instead ?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'm not sure how nested composite would behave but splitting the
setup
file would definitely make sense. Might be worth investigating that