-
Notifications
You must be signed in to change notification settings - Fork 37
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
feat(build): add build number #109
Conversation
78911c8
to
a66a817
Compare
a66a817
to
46768bc
Compare
await knex.raw(` | ||
UPDATE builds SET number = numbers.number | ||
FROM ( | ||
SELECT id, row_number() OVER (PARTITION BY "repositoryId" ORDER BY id) AS number |
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.
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.
Adding a test for the number would be good.
src/server/models/Build.js
Outdated
@@ -58,6 +59,16 @@ export default class Build extends BaseModel { | |||
} | |||
} | |||
|
|||
$toDatabaseJson() { | |||
const attributes = super.$toDatabaseJson() | |||
attributes.number = this.$knex().raw( |
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.
Does it work if we do a patch on the build?
c811966
to
0726b0d
Compare
knex = connect('test') | ||
await knex.migrate.latest() | ||
await truncateAll(knex) | ||
beforeAll(() => { |
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.
It's going to be much better 👍 .
Runs a function before any of the tests in this file run
I'm wondering if we could make it global.
src/server/models/Build.test.js
Outdated
@@ -13,6 +13,24 @@ const baseData = { | |||
describe('models/Build', () => { | |||
useDatabase() | |||
|
|||
describe('create build', () => { | |||
it('should add a build number', async () => { | |||
const build = await factory.create('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.
build1
?
src/server/models/Build.test.js
Outdated
expect(build.number).toBe(1) | ||
await build.$query().patch({ jobStatus: 'complete' }) | ||
expect(build.jobStatus).toBe('complete') | ||
}) |
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.
expect(build.number).toBe(1)
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.
A reload might be good too.
// Process on exit and signals. | ||
process.on('exit', () => { | ||
terminator() | ||
log('Node server stopped.') | ||
}); | ||
|
||
// Removed 'SIGPIPE' from the list - bugz 852598. |
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.
Looks like the issue was fixed nodejs/node-v0.x-archive#2286.
log('Shutdown server gracefully...') | ||
log(`${SHUTDOWN_TIMEOUT}ms before killing it.`) | ||
callbacks.forEach(callback => callback()) | ||
setTimeout(() => terminator(), SHUTDOWN_TIMEOUT) |
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.
In an ideal world, we would clear this timeout once the callbacks complete.
Right now, I'm expecting the program to always wait 3s before closing, with no feedback regarding if it was clean or not.
31ac82c
to
2fd7bf6
Compare
2fd7bf6
to
42a6b82
Compare
Closes #106