-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 Zoom: Fix authenticator bug and add missing fields #35369
Conversation
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,
|
…risto/zoom-fixes
@@ -206,15 +213,16 @@ definitions: | |||
type: SubstreamPartitionRouter | |||
parent_stream_configs: | |||
- stream: "#/definitions/meetings_list_tmp_stream" | |||
parent_key: "uuid" | |||
parent_key: "id" |
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.
The Zoom docs state that either a uuid or id can be provided. However, in my tests using the "uuid" in requests simply returned a "Meeting does not exist" message. By changing to "id" I was able to successfully fetch the records in our account.
action: IGNORE | ||
- type: DefaultErrorHandler | ||
partition_router: | ||
type: SubstreamPartitionRouter | ||
parent_stream_configs: | ||
- stream: "#/definitions/meetings_list_tmp_stream" | ||
parent_key: "uuid" | ||
parent_key: "id" |
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.
Same as comment above
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.
Looks good! But, the changes should be applied. LGTM once fixed.
What
The Zoom connector has been down for some time, and due to lacking a sandbox account the updates to the authenticator introduced in 1.0.0 were never published. A test account has now been set up, and we are able to at least verify the ability to connect successfully to the API and sync data for multiple streams. All
webinar
streams remain untested at this time, and we do not currently plan to add integration tests for these streams as this is a Community connector.How
Bugs
Schemas
meeting_list_tmp
andwebinar_list_tmp
parent streams.meeting_id
property inmeeting_registration_questions
streamQA
🚨🚨 User Impact
Users will be able to create connections and sync data again. This is a breaking change due to the new authenticator introduced in 1.0.0 and a retyped schema property. Users with existing connections will need to migrate by setting up their server-to-server apps in Zoom for the new authenticator, and refresh schemas if syncing data from the "meeting_registration_questions" stream.