Skip to content

Refactor migration ibmdb#38

Merged
ssh24 merged 1 commit intomasterfrom
refactor-migration-ibmdb
Mar 3, 2017
Merged

Refactor migration ibmdb#38
ssh24 merged 1 commit intomasterfrom
refactor-migration-ibmdb

Conversation

@b-admike
Copy link
Copy Markdown
Contributor

No description provided.

@ssh24 ssh24 force-pushed the refactor-migration-ibmdb branch from 5b24906 to 7be53ba Compare February 24, 2017 21:39
Comment thread lib/ibmdb.js Outdated
if (err) {
return done(Error(err));
}
// TODO: VALIDATE fields/indexes against model definition
Copy link
Copy Markdown
Contributor

@jannyHou jannyHou Mar 1, 2017

Choose a reason for hiding this comment

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

We can implement empty functions here if details are not provided.
So we can remove isActual here, use the one in base connector, and add two empty functions getAddModifyColumns and getColumnsToDrop here

@ssh24 ssh24 changed the title [DO NOT MERGE] Refactor migration ibmdb Refactor migration ibmdb Mar 2, 2017
@ssh24
Copy link
Copy Markdown
Contributor

ssh24 commented Mar 2, 2017

@slnode test please

@ssh24 ssh24 force-pushed the refactor-migration-ibmdb branch from 7be53ba to 396b710 Compare March 2, 2017 19:34
@ssh24
Copy link
Copy Markdown
Contributor

ssh24 commented Mar 2, 2017

@slnode test please

@ssh24 ssh24 requested review from loay and qpresley March 2, 2017 21:14
@jannyHou
Copy link
Copy Markdown
Contributor

jannyHou commented Mar 2, 2017

@ssh24 I think we need to create a new file migration.js in loopback-ibmdb then move all functions there. Instead of defining everything in ibmdb.js

Copy link
Copy Markdown
Collaborator

@qpresley qpresley left a comment

Choose a reason for hiding this comment

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

I really don't think these changes are valid. Not all databases that loopback-ibmdb is a dependency on support the same SQL statements for the functions being executed. For example, none of these will work in DB2z, DB2i or Informix. I think we need to re-think this move.

Copy link
Copy Markdown
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

LGTM

@ssh24 ssh24 force-pushed the refactor-migration-ibmdb branch from 8eafc71 to ba02b96 Compare March 3, 2017 20:12
Add alterTable as a base function

Move showFields and showIndexes function

Extract isActual

Take out isActual as it inherits from base connector
Add getAddModifyColumns and getDropColumns as empty fn

Extract migration functions to a file
@ssh24 ssh24 force-pushed the refactor-migration-ibmdb branch from ba02b96 to 072ad63 Compare March 3, 2017 20:12
Copy link
Copy Markdown
Contributor

@loay loay left a comment

Choose a reason for hiding this comment

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

LGTM

@ssh24 ssh24 merged commit 58ce604 into master Mar 3, 2017
@ssh24 ssh24 deleted the refactor-migration-ibmdb branch March 3, 2017 20:46
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.

5 participants