Skip to content
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

Raise Exception on precheck failures #28

Merged
merged 2 commits into from
Oct 9, 2018
Merged

Conversation

rhafer
Copy link
Contributor

@rhafer rhafer commented Sep 27, 2018

This introduces a custom RuntimeError Exception that is raised if
something goes wrong during the prechecks or migration.

Addtionally we make sure to use a non-zero exit code if any of the
prechecks fail. And it case of batch processings we stop the batch
now after finding errors in a database.

@@ -436,6 +443,9 @@ def main():
check_target_schema(db['target'])
cfg.CONF.command.func(cfg, db['source'], db['target'])

except Psql2MysqlRuntimeError as e:
print(e, file=sys.stderr)
sys.exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to distinguish exit codes from the other ones (e.g. from failure during migration)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. Would it help us much?

raise Psql2MysqlRuntimeError(
"Errors were found during prechecks. Please address the reported "
"issues manually and try again."
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still unsure if it might be better to exit upon first problem found. Instead of collection all problems of the database at once.

Copy link
Contributor

Choose a reason for hiding this comment

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

May be.
On the other hand, if you have many small errors that are easy to fix, it might be better to fix of them before running the tool again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I'll just leave it as is for now.

jsuchome
jsuchome previously approved these changes Oct 1, 2018
@rhafer
Copy link
Contributor Author

rhafer commented Oct 1, 2018

rebased and added unittest

Itxaka
Itxaka previously approved these changes Oct 2, 2018
Copy link
Contributor

@Itxaka Itxaka left a comment

Choose a reason for hiding this comment

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

Looks good although Im not sure why we are caching the error and not re-raising it again

dbdatamigrator.target_db = mock.MagicMock()
dbdatamigrator.target_db.getTables.return_value = {'foo': foo}
dbdatamigrator.target_db.getSortedTables.return_value = [foo]
with self.assertRaises(psql2mysql.Psql2MysqlRuntimeError):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also prefer the exception message was checked here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

This introduces a custom RuntimeError Exception that is raised if
something goes wrong during the prechecks or migration.

Addtionally we make sure to use a non-zero exit code if any of the
prechecks fail. And it case of batch processings we stop the batch
now after finding errors in a database.
If the target table does not exist throw an error before trying to
access it for setting up sqlalchemy type decorators.

Added unit test for this.
Copy link
Contributor

@s-t-e-v-e-n-k s-t-e-v-e-n-k left a comment

Choose a reason for hiding this comment

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

This looks great, I have one small nit that I don't want to block on.

@@ -100,3 +100,20 @@ def test_migrate(self):
dbdatamigrator.src_db.readTableRows.assert_called_with(foo)
dbdatamigrator.target_db.clearTable.assert_called_with(foo)
dbdatamigrator.target_db.writeTableRows.assert_called_once()

def test_migrate_target_missing(self):
chunk_size = 10
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: We don't need to set chunk_size or pass it the DbDataMigrator constructor

@rhafer rhafer merged commit eca3bd0 into SUSE:master Oct 9, 2018
@rhafer rhafer deleted the exitcode branch October 9, 2018 08:34
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.

None yet

4 participants