Conversation
* SAC-29455: Added new schema * SAC-29455: Updated child stream scheam to add parent id * Added sync, unitest cases and exception handling (#2) * SAC-29456:Updated sync, unitest cases and exception handling * SAC-29456: Addressed copilot review comments * SAC-29456: fixed integration tests * SAC-29456: Addressed review comments
There was a problem hiding this comment.
Pull request overview
This PR introduces the newly created tap-drip connector for extracting data from the Drip API. The implementation follows Singer.io specifications and includes comprehensive stream definitions, error handling, and test coverage.
Key Changes:
- Complete tap implementation with 15 data streams including accounts, conversions, campaigns, subscribers, workflows, and related entities
- Full test suite including unit tests and integration tests
- CI/CD configuration with CircleCI pipeline
Reviewed changes
Copilot reviewed 57 out of 58 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tap_drip/init.py | Main entry point implementing discovery and sync modes |
| tap_drip/client.py | HTTP client with authentication, error handling, and retry logic |
| tap_drip/sync.py | Sync orchestration managing stream selection and state |
| tap_drip/streams/abstracts.py | Base stream classes defining replication patterns |
| tap_drip/streams/*.py | Individual stream implementations for all Drip API endpoints |
| tap_drip/schemas/*.json | JSON schemas for all supported streams |
| tests/unittests/*.py | Unit tests for sync, client, and bookmark functionality |
| tests/test_*.py | Integration tests for discovery, pagination, bookmarks, etc. |
| setup.py | Package configuration with dependencies |
| .circleci/config.yml | CI pipeline configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/test_discovery.py
Outdated
| logging=f"verify {expected_parent_tap_stream_id} " | ||
| f"is saved in metadata as a parent-tap-stream-id") |
There was a problem hiding this comment.
The parameter name 'logging' is misleading. This appears to be an assertion message parameter, which should be named 'msg' to match unittest conventions.
| logging=f"verify {expected_parent_tap_stream_id} " | |
| f"is saved in metadata as a parent-tap-stream-id") | |
| msg=f"verify {expected_parent_tap_stream_id} " | |
| f"is saved in metadata as a parent-tap-stream-id") |
README.md
Outdated
|
|
||
| **[tags](https://developer.drip.com/?shell#list-all-tags-used-in-an-account)** | ||
| - Data Key = tags | ||
| - Primary keys: ['tag_name', 'account_id'] |
There was a problem hiding this comment.
Primary key mismatch: README documents 'tag_name' as primary key for tags stream, but tests/base.py line 125 and tap_drip/streams/tags.py line 5 both define 'tag_id' as the primary key. Update README to use 'tag_id'.
| - Primary keys: ['tag_name', 'account_id'] | |
| - Primary keys: ['tag_id', 'account_id'] |
spike/tap-drip-config.json
Outdated
| "event_actions", | ||
| "forms", | ||
| "single_email_campaigns", | ||
| "peoples", |
There was a problem hiding this comment.
Stream name 'peoples' is inconsistent with the actual stream name 'people' used throughout the codebase (tap_drip/streams/people.py line 4, README.md line 29). Should be 'people'.
| "peoples", | |
| "people", |
README.md
Outdated
|
|
||
| **[custom_field_identifiers](https://developer.drip.com/?shell#list-all-custom-field-identifiers-used-in-an-account)** | ||
| - Data Key = custom_field_identifiers | ||
| - Primary keys: ['id', 'account_id'] |
There was a problem hiding this comment.
Primary key mismatch for custom_field_identifiers: README shows ['id', 'account_id'] but tests/base.py line 53 and tap_drip/streams/custom_field_identifiers.py line 5 define ['custom_field_id', 'account_id']. Update README to match implementation.
| - Primary keys: ['id', 'account_id'] | |
| - Primary keys: ['custom_field_id', 'account_id'] |
spike/tap-drip-config.json
Outdated
| { | ||
| "name": "custom_field_identifiers", | ||
| "key_properties": [ | ||
| "id", |
There was a problem hiding this comment.
Primary key mismatch: config shows ['id', 'account_id'] for custom_field_identifiers, but the actual implementation uses ['custom_field_id', 'account_id'] (tap_drip/streams/custom_field_identifiers.py line 5). Update config to match implementation.
| "id", | |
| "custom_field_id", |
spike/tap-drip-config.json
Outdated
| { | ||
| "name": "tags", | ||
| "key_properties": [ | ||
| "tag_name", |
There was a problem hiding this comment.
Primary key mismatch: config uses 'tag_name' but the implementation uses 'tag_id' (tap_drip/streams/tags.py line 5, tests/base.py line 125). Update config to use 'tag_id'.
| "tag_name", | |
| "tag_id", |
|
why build is failing? |
|
Integration tests are passing. |
.circleci/config.yml
Outdated
| - master | ||
| jobs: | ||
| - build: | ||
| context: circleci-user No newline at end of file |
* Add retry logic for server 5xx errors * Addressed copilot review comments * Remove 5xx errors from exceptions per API docs * Update logic to use single dictionary lookup * Exclude streams with less records than API_LIMIT * Added logging when using default retry
Co-authored-by: atttiwari <atul.tiwari@qlik.com>
| @@ -0,0 +1,2 @@ | |||
|
|
|||
There was a problem hiding this comment.
Add version number :
#0.0.1
setup.py
Outdated
|
|
||
|
|
Description of change
SAC-29457
Consolidated PR to merge the newly created tap-drip.
Manual QA steps
Notes
Rollback steps