-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Twilio migration to low code. #40248
Conversation
The latest updates on your projects. Learn more about Vercel for Git βοΈ
|
cristina.mariscal seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
53f6da0
to
eb7382d
Compare
c848546
to
560e9a4
Compare
β¦m-airbyte/twilioLCAccounts
β¦m-airbyte/twilioLCAccounts
β¦m-airbyte/twilioLCAccounts
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration 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.
Testing Manually
I have only tested the streams mentioned below although there are many more. I was using our sandbox account. Can we investigate the ones marked as to be investigated?
accounts
[Accepted change] Pagination has been removed from the account streams which makes request go from https://api.twilio.com/2010-04-01/Accounts.json?PageSize=1000
to https://api.twilio.com/2010-04-01/Accounts.json?
addresses
OK
available_phone_number_countries
OK
available_phone_numbers_local
[to be investigated] Getting Not found. The requested resource was not found on the server
when the connector queries https://api.twilio.com/2010-04-01/Accounts/****/AvailablePhoneNumbers/BE/Local.json?PageSize=1000
available_phone_numbers_mobile
[to be investigated] Getting Not found. The requested resource was not found on the server
when the connector queries https://api.twilio.com/2010-04-01/Accounts/****/AvailablePhoneNumbers/AR/Mobile.json?PageSize=1000
available_phone_numbers_toll-Free
[to be investigated] This stream was added in the manifest version but does not exist in the current version
incoming_phone_numbers
OK
keys
OK
flows
OK
step
[to be investigated] The latest version produces two records while the manifest one does not have any record. While reading through the logs of the manifest version output, I can see The requested resource /Flows/FW7ad717a690629a6da33bd3c8b9cf7d97/Executions/FN9715cbc94b32ed1090874270ad72efb7/Steps was not found
alerts
[to be investigated] There is a mismatch between the records. I get 4 from the latest version and 6 from the CDK one. Trying to match the first one of the latest version, I can't match 100% with any of the manifest one.
applications
OK
dependent_phone_numbers
[to be investigated] I get The requested resource /2010-04-01/Accounts/AC4cac489c46197c9ebc91c840120a4dee/Addresses/AD0164001bc0f84d9bc29e17378fe47c20/DependentPhoneNumbers.json was not found
with the manifest version
β¦m-airbyte/twilioLCAccounts
β¦m-airbyte/twilioLCAccounts
} | ||
} | ||
} | ||
``` |
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.
Could you please add steps how to migrate to new version for the users? for example refresh source schema + clear data for affected streams. You can find an example in migration guide for other connectors(tiktok, slack)
breakingChanges: | ||
1.0.0: | ||
message: This release changes the state format of existing connections. | ||
upgradeDeadline: "2024-12-01" |
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.
Please add scopedImpact
here
- $ref: "#/definitions/streams/Users" | ||
- $ref: "#/definitions/streams/User Conversations" | ||
|
||
spec: |
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 have spec in manifest
and in spec.json
, should we delete one of them?
definitions: | ||
base_requester: | ||
type: HttpRequester | ||
url_base: https:// |
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.
should we add https://api.twilio.com/2010-04-01
here to url_base, instead of defining it in requester.path
for each stream?
type: MinMaxDatetime | ||
datetime: "{{ now_utc().strftime('%Y-%m-%dT%H:%M:%SZ') }}" | ||
datetime_format: "%Y-%m-%dT%H:%M:%SZ" | ||
cursor_granularity: P1D |
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.
should cursor_granularity
be 1s in this case because we have datetime format with seconds precision? doesn't P1D
lead to missing records?
- type: AddedFieldDefinition | ||
path: | ||
- date_created | ||
value: '{% set formatted_datetime = format_datetime(record["date_created"], "%Y-%m-%dT%H:%M:%SZ", "%a, %d %b %Y %H:%M:%S %z") %}{{ formatted_datetime }}' |
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.
Just a question, what was the reason to change the format of date related fields?
fields: | ||
- type: AddedFieldDefinition | ||
path: | ||
- duration |
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.
You can add schema_normalization: Default
to record selector and it will autotransform fields like this into type from the schema, so you don't need to transform them here.
@girarda if you'd like, community devs team would love to help out with any remaining low-code migrations, including Twilio. I think @pabloescoder will have capacity soon. |
@natikgadzhi @girarda Sure! I would certainly love to pick up any low code migrations including Twilio. Please let me know if there are any I could take up. |
@pabloescoder wait for @girarda's blessing before getting started |
@natikgadzhi Haha I'll wait π |
Hello @pabloescoder, sorry for the delays. thank you for offering! your help would be much welcomed here! |
No problem! Apologies for the late reply. I'll look into it right away! @girarda |
@pabloescoder I've put this on our project board and set the priority. Still important to get to! |
Sorry! This slipped out of my radar, will pick it up soon |
@pabloescoder taking after the quickbooks tasks? |
@natikgadzhi Yes! Let's wrap up with quickbooks by this weekend or early next week, then I'll start with this. It's been delayed for a long time. |
@pabloescoder this is still valid to work on, but you probably can use this as reference and start from scratch. Also, we should go to manifest-only and just make a schema-close-to-compatible version from scratch. |
What & how
Migrating Twilio connector to low code.
Review guide
User Impact
This must be transparent for users.
Can this PR be safely reverted and rolled back?