Added new connector: source-twilio #1365
Conversation
Setup for base image snapshot collection Added initial snapshots copying the base image behaviour NIT: quick fix so the pull wont conflict with poetry
Remote Repo URL: git@github.com:airbytehq/airbyte.git Source name: 8c862a8 Source Commit ID: 8c862a8013150cfee238c379e66ff9e84550da6b Source Repo Prefix: airbyte-integrations/connectors/source-twilio/ Import Path: source-twilio/ License Type: MIT License Path: airbyte-integrations/connectors/source-twilio/metadata.yaml git-merge-subpath: 8c862a8013150cfee238c379e66ff9e84550da6b airbyte-integrations/connectors/source-twilio source-twilio
Luishfs
force-pushed
the
backup/source-twilio
branch
3 times, most recently
from
b7cf98a to
39655da
Compare
Luishfs
requested review from
williamhbaker
and removed request for
jgraettinger
NOTE: a small amount of fixups were necessary because this connector is still MIT Added to CI
Luishfs
force-pushed
the
backup/source-twilio
branch
from
39655da to
6fd0254
Compare
|
There are a couple more recent commits to |
@williamhbaker I believe they were added last week, so i missed then on the import :) . Is our image in sync with those changes? If so i can force push the new import and roll all changes again |
Ah I see. Our strategy has been to import the latest we coudl that is still MIT licensed, so it would be good to update our import since we have the opportunity to do so. As far as our image being in sync with those changes, assuming you are referring to the connector running on ATF I'd guess it isn't. But still, we want to get the latest imported version we can, understanding that it may be a bit more work for us to make it backward compatible, because the latest updates to airbyte tend to be useful and backward compatible out of the box. |
There was a problem hiding this comment.
We need snapshot tests comparing the connector output pre and post conversion if at all possible, and also in the PR description describe how you tested the migration from the old version of the connector (pre-CDK) to the new version built from this branch.
See other inline comments as well
| @@ -880,19 +953,13 @@ | |||
| ] | |||
| } | |||
| }, | |||
| "required": [ | |||
There was a problem hiding this comment.
The collection key needs to be required, or materializations will have a bad time materializing the collection generally.
| @@ -1004,19 +1085,13 @@ | |||
| ] | |||
| } | |||
| }, | |||
| "required": [ | |||
There was a problem hiding this comment.
Collection key should be required, see above
| @@ -1354,10 +1459,6 @@ | |||
| ] | |||
| } | |||
| }, | |||
| "required": [ | |||
There was a problem hiding this comment.
Collection keys should be required, see above, and assume this applies for all other collection keys as well, since I'm going to stop noting this for each individual collection as I continue reading through the snapshot 😄
There was a problem hiding this comment.
Got it, going to update this
| "type": [ | ||
| "null", | ||
| "string" | ||
| ] | ||
| }, | ||
| "price": { |
There was a problem hiding this comment.
I believe that was due to all changes from the commits and the ATF disparity with the version i imported.
Just checked it, and the source-twillio image has been updated yesterday, so im going to pull the current commit and start again. Hopefully we will remove most of this noise
There was a problem hiding this comment.
Will also provide the jsondiff link.
|
As mentioned earlier, im removing this PR and moving it here, the new branch is created inside estuary/connectors and all other comments were already adressed |
Description:
(Describe the high level scope of new or changed features)
Notes for reviewers:
(anything that might help someone review this PR)
This change is