-
Notifications
You must be signed in to change notification settings - Fork 1
HGI-6804: add target_state #7
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
base: master
Are you sure you want to change the base?
Conversation
| @@ -1,41 +1,46 @@ | |||
| #!/usr/bin/env python | |||
|
|
|||
| from setuptools import setup | |||
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.
Why do we have changes in this file?
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.
Fixed
|
|
||
| try: | ||
| import tests.utils as test_utils | ||
| import tests.utils as test_utils # type: ignore |
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.
Why do we have changes in the tests folder?
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.
Fixed
target_postgres/__init__.py
Outdated
| batch_summary["updated"] += updated_lines | ||
| except Exception as e: | ||
| for batch_bookmark in batch_bookmarks: | ||
| batch_bookmark["reason"] = str(e) |
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 think this should actually be "error" – I didn't give you a good example for that, sorry
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.
Fixed
target_postgres/__init__.py
Outdated
| "hash": build_record_hash(record), | ||
| "success": True, | ||
| "external_id": record.get('cid',""), | ||
| "reason": "" |
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.
We don't need this reason key
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.
Fixed
target_postgres/__init__.py
Outdated
| "hash": build_record_hash(record), | ||
| "success": False, | ||
| "external_id": record.get('cid',""), | ||
| "reason": str(e) |
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.
Same feedback as below, this should be "error"
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.
Fixed
target_postgres/__init__.py
Outdated
| batch_state["bookmarks"][stream].append({ | ||
| "hash": build_record_hash(record), | ||
| "success": False, | ||
| "external_id": record.get('cid',""), |
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.
What is cid in this context? Is it guaranteed to always exist or was it just an example from your data.singer? In other targets we check if the user has supplied externalId and if not we don't include this external_id in the bookmark for this record
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 was particular to the data.singer. Fixed to check if externalId was supplied.
No description provided.