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

fix bug where the before_commit hook runs before the after_begin hook #17

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

wjordan213
Copy link

Merge the before commit and after begin hooks due to the fact that they can happen out of order.

This addresses an issue that I was having when testing the DMR project locally. I observed rows being added to the audit transactions table that did not contain a value for origin_app. Implementing this change in the library code running inside of the docker container executing the data migration fixed the issue that I was having.

@wjordan213
Copy link
Author

PR that introduced issue here: https://github.com/trialspark/sqlalchemy-continuum/pull/14/files

@wjordan213
Copy link
Author

Figuring out how to run tests against this repo now

@wjordan213 wjordan213 force-pushed the wjordan213__PLATFORM-943__merge_after_begin_and_before_commit_hooks branch from 42562b8 to 308f203 Compare July 27, 2021 20:33
Copy link

@rmurcek rmurcek left a comment

Choose a reason for hiding this comment

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

LGTM assuming the continuum tests pass and the spark tests pass when pointed at this version!

Copy link
Collaborator

@minznerjosh minznerjosh left a comment

Choose a reason for hiding this comment

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

I think this is fine? Have you run all the automated tests for contiuum? (We don't have CI set up for this repo so you'll need to do it locally.)

@wjordan213
Copy link
Author

yeah were the tests running properly before? I'm unable to run any tests against even master using:
tox -e py35 (3.5 is the latest version we have listed on this repo?!)

@wjordan213
Copy link
Author

getting random schema errors and things

@wjordan213
Copy link
Author

@minznerjosh if I ran this against the spark repo's codebase would that be sufficient?

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.

3 participants