-
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
🚨🚨 Source Microsoft Teams: Update Schemas #33959
Conversation
…risto/microsoft-teams-cdk
…risto/microsoft-teams-cdk
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Before Merging a Connector Pull RequestWow! What a great pull request you have here! 🎉 To merge this PR, ensure the following has been done/considered for each connector added or updated:
If the checklist is complete, but the CI check is failing,
|
"type": ["null", "string"] | ||
}, | ||
"user_principal_name": { | ||
"deletedDate": { | ||
"type": ["null", "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.
Did not add a "date" format to the "deletedDate" field; it is expressed as an empty string rather than a null value when empty, which will throw a validation error if the format is enforced.
@alafanechere - what do you think about this CAT test failure? Should we allow this case? |
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.
@alafanechere - what do you think about this CAT test failure? Should we allow this case?
I indeed think that the additionalProperties
field should be removed from the credentials
object in the spec, it will default to true
.
If we're not allowing additionalProperties it means we'd have to perform a config migration on the next iteration on this credentials
field if we remove a field and a rollback to the previous config state needs to be supported in case of a version downgrade.
So we enforce additionalProperties: true
on spec field to keep flexibility and maximize backward compatiblity.
Looking at this PR, this is a test that was likely previously failing. And to make it pass we should just remove additionalProperties: false
and the credentials
oneOf (not the advancedAuth
section.
…risto/microsoft-teams-cdk
@alafanechere Thanks for taking a look at this! Apologies for the slow turnaround, took me a while to figure out what was causing the CAT failures when removing the It turns out the root cause was just a slight misconfiguration in our GSM credentials which was preventing the connector from validating the correct auth method being used when additional properties were allowed. I've updated the creds, and we are now fully green with additionalProps allowed 👍 |
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.
🟢 CI = LGTM 😄
What
This PR is part of the larger effort to update the CDK versions on some of our older Community connectors. It also resolves #10243.
anyOf
keyword being used to make array fields nullable, in favor of:date
andtimestamp_without_timezone
airbyte-types where applicable.tenant_id
field as an airbyte_secret in the connector's specconfigured_catalog.json
airbyte-cdk
versionNote on CAT failure
There is one outstanding CAT test failure due to the explicit declaration of
additionalProperties: false
in the connector's spec. We enforce that our specs should never explicitly disallow additional properties for backwards compatibility purposes, but it seems in this connector the flag is being purposefully used to delineate two authentication options (OAuth and Token) which are almost identical in nature:Removing the
false
flag causes the OAuthcheck
to fail, due to the credentials being considered valid for either authentication method (sincerefresh_token
is treated as a valid undeclared key for Token auth). While this reliance on an anti-pattern is not ideal, it seems to work, so I think a fix is outside the scope of this update and best saved for a separate PR. I have created an issue and am hoping this PR can be merged in spite of the test failure.Suggested Reading Order
User Impact
This is a breaking change due to the removal/renaming of multiple schema properties. Although Microsoft Teams does not support incremental sync modes, customers will still need to refresh their schemas, and should be encouraged to reset their data if they are using
Full Refresh | Append
mode.