-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Small spec fixes to make sure they work with connector form UI #21587
Conversation
/test connector=connectors/source-amazon-ads
Build FailedTest summary info:
|
/test connector=connectors/source-linkedin-ads
Build FailedTest summary info:
|
/test connector=connectors/source-s3
Build PassedTest summary info:
|
/test connector=connectors/source-sendgrid
Build FailedTest summary info:
|
/test connector=connectors/source-zoho-crm
Build FailedTest summary info:
|
/test connector=connectors/source-amazon-ads
Build FailedTest summary info:
|
/test connector=connectors/source-linkedin-ads
Build PassedTest summary info:
|
/test connector=connectors/source-sendgrid |
@@ -51,7 +51,7 @@ | |||
"order": 6, | |||
"type": "array", | |||
"items": { | |||
"type": "integer" | |||
"type": "string" |
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 did a quick check in the prod db and these items appear to be integers so we'll need to migrate existing cloud configurations
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.
Thanks for flagging this - I wondered how these configs got created and realized that I missed the “array of integers” support in the UI which means I can revert this change. I will update this PR and the SAT extension.
@girarda For the sendgrid one I checked and right now users have to use the integer format (specifying a unix timestamp). So we should run a migration for these. Alternatively we could just take away the string format (because so far users can't use that), but it seems like only allowing strings for a start time is a much better choice than only allowing unix timestamps like we effectively do right now. |
/test connector=connectors/source-sendgrid
Build PassedTest summary info:
|
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.
approved assuming the change is S3 is not breaking
@@ -17,5 +17,5 @@ COPY source_s3 ./source_s3 | |||
ENV AIRBYTE_ENTRYPOINT "python /airbyte/integration_code/main.py" | |||
ENTRYPOINT ["python", "/airbyte/integration_code/main.py"] | |||
|
|||
LABEL io.airbyte.version=0.1.28 | |||
LABEL io.airbyte.version=0.1.29 |
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 this be a minor update / is this a breaking change?
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.
no, that's actually a patch change - it doesn't change what's allowed for this field, it just makes sure the UI understands correctly.
/test connector=connectors/source-sendgrid
Build PassedTest summary info:
|
/test connector=connectors/source-s3
Build PassedTest summary info:
|
/publish connector=connectors/source-sendgrid
if you have connectors that successfully published but failed definition generation, follow step 4 here |
/publish connector=connectors/source-s3
if you have connectors that successfully published but failed definition generation, follow step 4 here |
Airbyte Code Coverage
|
#21451 is making the SATs more strict w.r.t. UI limitations - some schemas are valid JSON schema, but will cause small issues in the UI. This PR is fixing the connectors causing issues:
s3
Enum was encoded within an
allOf
(see pydantic/pydantic#2088) - added a rewrite logic to fix the schema so it works correctly in the UI.As this changed the previous schema I had to disable the compatibility check
sendgrid
Date had either integer or string set as value - the UI will always produce strings anyway, reflected this in the schema