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.

Sign up for GitHub

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

Jump to bottom

🎉 New source: airbyte-native twilio connector #4070

Merged
merged 26 commits into from
Jul 3, 2021
Merged

🎉 New source: airbyte-native twilio connector #4070

merged 26 commits into from
Jul 3, 2021

Conversation

htrueman
Copy link
Contributor

@htrueman htrueman commented Jun 12, 2021 •
edited
Loading

What

Closes #3697

How

  • Added native Twilio source integration.
  • Implemented all streams available present in singer tap.

Recommended reading order

  1. Schemas and configs in source_twilio/schemas/ and integration_tests/
  2. source_twilio python modules.
  3. Docs and source definitions in source root and common docs dir.
  4. the rest

Some important notes:

  • I could not fill dependent_phone_numbers stream.

  • conference_participants has records only if there are active conference participants (participants, which on conference call at the moment request is made).

  • Some streams have values which changes on each read, so I created a separate catalog for full refresh tests (constant_records_catalog.json).

  • conferences stream is iterable, but cursor field is date-time field in record and filter param is date field. This causes that, for instance, cursor value is saved as 2020-01-01T10:10:10Z, but during filtering it's reduced to date 2020-01-01, and we'll receive records between 2020-01-01T00:00:00Z and 2020-01-02T00:00:00Z, thus tests fail.

    It may be fixed if we update parse_value in json_schema_helper.py to support date type similar to date-time.

    So another catalog no_empty_streams_no_conferences_catalog.json created for incremental tests.
    Tap source has incremental support for a bunch more streams, but that is just client side filtering after full read, so I cut it out.

Pre-merge Checklist

  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • Secrets are annotated with airbyte_secret in output spec
  • Unit & integration tests added as appropriate (and are passing)
    • Community members: please provide proof of this succeeding locally e.g: screenshot or copy-paste acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • /test connector=connectors/<name> command as documented here is passing.
    • Community members can skip this, Airbyters will run this for you.
  • Code reviews completed
  • Credentials added to Github CI if needed and not already present. instructions for injecting secrets into CI.
  • Documentation updated
    • README
    • CHANGELOG.md
    • Reference docs in the docs/integrations/ directory.
    • Build status added to build page
  • Build is successful
  • Connector version bumped like described here
  • New Connector version released on Dockerhub by running the /publish command described here
  • No major blockers
  • PR merged into master branch
  • Follow up tickets have been created
  • Associated tickets have been closed & stakeholders notified

yevhenii-ldv
yevhenii-ldv requested changes Jun 15, 2021
View reviewed changes
Copy link
Contributor

@yevhenii-ldv yevhenii-ldv left a comment

Choose a reason for hiding this comment

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

Look very good, just a few comments.

@yevhenii-ldv
Copy link
Contributor

@htrueman And one more clarification, I did not check everything, but it looks like you just copied the schemas from Singer Tap. Are you sure Singer hasn't altered the data received from the API in any way? Because I did not see any transformations from you, and you left Singer's schemes.

@yevhenii-ldv
Copy link
Contributor

@htrueman please, run tests)

@github-actions github-actions bot added the area/connectors Connector related issues label Jun 18, 2021
@htrueman
Copy link
Contributor Author

htrueman commented Jun 18, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-twilio

🕑 connectors/source-twilio https://github.com/airbytehq/airbyte/actions/runs/950608984
❌ connectors/source-twilio https://github.com/airbytehq/airbyte/actions/runs/950608984

@htrueman
Copy link
Contributor Author

Some important notes:

  • I could not fill dependent_phone_numbers stream.

  • conference_participants has records only if there are active conference participants (participants, which on conference call at the moment request is made).

  • Some streams have values which changes on each read, so I created a separate catalog for full refresh tests (constant_records_catalog.json).

  • conferences stream is iterable, but cursor field is date-time field in record and filter param is date field. This causes that, for instance, cursor value is saved as 2020-01-01T10:10:10Z, but during filtering it's reduced to date 2020-01-01, and we'll receive records between 2020-01-01T00:00:00Z and 2020-01-02T00:00:00Z, thus tests fail.

    It may be fixed if we update parse_value in json_schema_helper.py to support date type similar to date-time.

    So another catalog no_empty_streams_no_conferences_catalog.json created for incremental tests.

  • Tap source has incremental support for a bunch more streams, but that is just client side filtering after full read, so I cut it out.

@sherifnada
Copy link
Contributor

@davinchia @tuliren would either of you be able to take a look?

@davinchia
Copy link
Contributor

Happy to pick it up, but won't get to this until tmrw.

davinchia
davinchia reviewed Jul 2, 2021
View reviewed changes
airbyte-integrations/connectors/source-twilio/source_twilio/streams.py
"""https://www.twilio.com/docs/voice/api/call-resource#create-a-call-resource"""

parent_stream = Accounts
incremental_filter_field = "EndTime>"
Copy link
Contributor

@davinchia davinchia Jul 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

why is there a > at the end of this? is this part of Twilio's API?

Copy link
Contributor

Choose a reason for hiding this comment

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

confused since I see a mixture of incremental_filter_fields with > and without.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree that is confusing, but it is the only way Twilio filtering works. For instance, https://www.twilio.com/docs/usage/api/usage-record#read-multiple-usagerecord-resources support only filters like StartDate=YYYY-MM-DD, but the https://www.twilio.com/docs/voice/api/call-resource#create-a-call-resource like StartTime>YYYY-MM-DD (or StartTime<YYYY-MM-DD, which is not needed for our purposes).

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks for the explanation. Can you also leave a comment the first time this appears? Something like `This is how Twilio filtering works. See for more info.'

davinchia
davinchia requested changes Jul 2, 2021
View reviewed changes
Copy link
Contributor

@davinchia davinchia left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@htrueman nice work! appreciate how detail oriented you are and the extra tests you added.

I have some minor question and comments before I approve this.

Can you also go through the pre-merge checklist? Particularly the documentation section, we need to make sure all the right documetation is updated. This can be done in this PR or a follow up PR. My preference is with this PR.

conferences stream is iterable, but cursor field is date-time field in record and filter param is date field. This causes that, for instance, cursor value is saved as 2020-01-01T10:10:10Z, but during filtering it's reduced to date 2020-01-01, and we'll receive records between 2020-01-01T00:00:00Z and 2020-01-02T00:00:00Z, thus tests fail.

What's the update here? I see the tests passing so I assume we figured something out? If we don't we should create a follow up ticket to address this.

@htrueman
Copy link
Contributor Author

htrueman commented Jul 2, 2021 •
edited
Loading

@htrueman nice work! appreciate how detail oriented you are and the extra tests you added.

I have some minor question and comments before I approve this.

Can you also go through the pre-merge checklist? Particularly the documentation section, we need to make sure all the right documetation is updated. This can be done in this PR or a follow up PR. My preference is with this PR.

conferences stream is iterable, but cursor field is date-time field in record and filter param is date field. This causes that, for instance, cursor value is saved as 2020-01-01T10:10:10Z, but during filtering it's reduced to date 2020-01-01, and we'll receive records between 2020-01-01T00:00:00Z and 2020-01-02T00:00:00Z, thus tests fail.

What's the update here? I see the tests passing so I assume we figured something out? If we don't we should create a follow up ticket to address this.

We indeed found a solution for that:

  • SAT was updated in another PR, so now date cursors are also supported.
  • In Conferences.parse_response() method I do filter the records, to exclude the unnecessary entries.
  • This filtering would affect only the records within a 24 hours, so may slow down our reading and not take any additional requests.

UPD: Source docs are also added. Pre-merge Checklist filled.

@davinchia davinchia removed the request for review from sherifnada July 2, 2021 13:00
@htrueman
Update streams code style.
51a8b96 
Add twilio.md docs.
Other PR style fixes.
@htrueman
Update twilio.md docs.
8c41223
@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Jul 2, 2021
@htrueman
Copy link
Contributor Author

htrueman commented Jul 2, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-twilio

🕑 connectors/source-twilio https://github.com/airbytehq/airbyte/actions/runs/993772220
✅ connectors/source-twilio https://github.com/airbytehq/airbyte/actions/runs/993772220

@htrueman htrueman requested a review from davinchia July 2, 2021 13:38
davinchia
davinchia approved these changes Jul 2, 2021
View reviewed changes
Copy link
Contributor

@davinchia davinchia left a comment

Choose a reason for hiding this comment

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

1 minor comment. Feel free to merge after addressing it. Please also make sure the tests are properly passing before doing so. Nice!

@htrueman
Copy link
Contributor Author

htrueman commented Jul 3, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-twilio

🕑 connectors/source-twilio https://github.com/airbytehq/airbyte/actions/runs/996663427
❌ connectors/source-twilio https://github.com/airbytehq/airbyte/actions/runs/996663427
🕑 connectors/source-twilio https://github.com/airbytehq/airbyte/actions/runs/996663427
❌ connectors/source-twilio https://github.com/airbytehq/airbyte/actions/runs/996663427
🕑 connectors/source-twilio https://github.com/airbytehq/airbyte/actions/runs/996663427
❌ connectors/source-twilio https://github.com/airbytehq/airbyte/actions/runs/996663427
🕑 connectors/source-twilio https://github.com/airbytehq/airbyte/actions/runs/996663427
❌ connectors/source-twilio https://github.com/airbytehq/airbyte/actions/runs/996663427

@htrueman
Copy link
Contributor Author

htrueman commented Jul 3, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-twilio

🕑 connectors/source-twilio https://github.com/airbytehq/airbyte/actions/runs/996741584
✅ connectors/source-twilio https://github.com/airbytehq/airbyte/actions/runs/996741584

@htrueman htrueman merged commit 9531c4d into master Jul 3, 2021
@htrueman htrueman deleted the htrueman/airbyte-native-twilio-connector branch July 3, 2021 17:59
@htrueman htrueman mentioned this pull request Jul 5, 2021
Merged
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keu keu keu approved these changes

@davinchia davinchia davinchia approved these changes

@Zirochkaa Zirochkaa Zirochkaa approved these changes

@vitaliizazmic vitaliizazmic vitaliizazmic approved these changes

@yevhenii-ldv yevhenii-ldv Awaiting requested review from yevhenii-ldv

Assignees
No one assigned
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

new source: airbyte-native twilio connector
7 participants
@htrueman @yevhenii-ldv @keu @sherifnada @davinchia @Zirochkaa @vitaliizazmic