Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new source connector for Calendly, a scheduling tool. The connector implements both incremental and snapshot synchronization strategies to capture various Calendly entities including event types, scheduled events, invitees, routing forms, and organization memberships. The implementation follows the Estuary CDK patterns and includes comprehensive test coverage with snapshot tests.
Changes:
- Adds a new Calendly source connector with support for 7 different entity types
- Implements incremental sync for event_types and snapshot sync for other entities
- Includes child entity support for event_invitees and routing_form_submissions
Reviewed changes
Copilot reviewed 21 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| source-calendly/source_calendly/init.py | Main connector class implementing spec, discover, validate, and open methods |
| source-calendly/source_calendly/models.py | Data models for Calendly entities and endpoint configuration |
| source-calendly/source_calendly/api.py | API interaction logic including pagination, backfill, and incremental fetch |
| source-calendly/source_calendly/resources.py | Resource definitions and credential validation |
| source-calendly/source_calendly/main.py | Entry point for running the connector |
| source-calendly/pyproject.toml | Project dependencies and build configuration |
| source-calendly/config.yaml | Encrypted connector configuration |
| source-calendly/test.flow.yaml | Test Flow configuration with all resource bindings |
| source-calendly/tests/test_snapshots.py | Snapshot tests for capture, discover, and spec |
| source-calendly/tests/snapshots/*.json | Snapshot test outputs for validation |
| source-calendly/acmeCo/*.schema.yaml | JSON schemas for all collection types |
| source-calendly/acmeCo/flow.yaml | Collection definitions with keys |
| source-calendly/VERSION | Version file marking v1 release |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| async def validate_credentials( | ||
| http: HTTPMixin, config: EndpointConfig, log: Logger |
There was a problem hiding this comment.
The parameter order for validate_credentials should be (log, http, config) to match the established convention in other connectors in this repository. Both source-shopify-native and source-klaviyo-native follow the pattern of having log as the first parameter, http as the second, and config as the third. The call site in init.py should also be updated accordingly.
| http: HTTPMixin, config: EndpointConfig, log: Logger | |
| log: Logger, http: HTTPMixin, config: EndpointConfig |
| log: Logger, | ||
| validate: request.Validate[EndpointConfig, ResourceConfig], | ||
| ) -> response.Validated: | ||
| await validate_credentials(self, validate.config, log) |
There was a problem hiding this comment.
The call to validate_credentials should be updated to match the established parameter order convention: await validate_credentials(log, self, validate.config). This aligns with the pattern used in other connectors like source-shopify-native and source-klaviyo-native.
| await validate_credentials(self, validate.config, log) | |
| await validate_credentials(log, self, validate.config) |
| return Request[EndpointConfig, ResourceConfig, ConnectorState] | ||
|
|
||
| @override | ||
| async def spec(self, _: request.Spec, logger: Logger) -> ConnectorSpec: |
There was a problem hiding this comment.
The spec method's parameter order should match the established convention: 'log: Logger' should come before the request parameter, not after. All other native connectors (Shopify, Klaviyo, HubSpot) use 'async def spec(self, log: Logger, _: request.Spec)'. Additionally, the parameter should be named 'log', not 'logger', for consistency.
| async def spec(self, _: request.Spec, logger: Logger) -> ConnectorSpec: | |
| async def spec(self, log: Logger, _: request.Spec) -> ConnectorSpec: |
cfbcbe1 to
7f47a9d
Compare
| from estuary_cdk.flow import AccessToken | ||
| from pydantic import AwareDatetime, BaseModel, Field | ||
|
|
||
| API_BASE_URL = "https://api.calendly.com" |
There was a problem hiding this comment.
Though usually defined in api.py, this time it needs to stay on models.py to avoid a circular dependency error
7f47a9d to
fd039ba
Compare
Alex-Bair
left a comment
There was a problem hiding this comment.
LGTM % binding only the required fields instead of the entire config to various fetch_changes / fetch_page functions.
There was a problem hiding this comment.
nit: could we rebase this branch onto the recently merged changes to estuary-cdk's dependencies in 5ca35fb then regenerate poetry.lock? I suspect a good number of dependencies will be removed since airbyte-cdk is now an optional dependency.
| assert result.returncode == 0 | ||
| lines = [json.loads(l) for l in result.stdout.splitlines()] | ||
|
|
||
| FIELDS_TO_REDACT = [] |
There was a problem hiding this comment.
nit: to save some snapshot churn in the future, it'd be good to redact updated_at and other other datetime fields that might change for reasons unrelated to connector code (ex: someone logs into the Calendly account, Calendly automatically updates a records, etc.)
There was a problem hiding this comment.
Right, I should be more proactive about this. I've inspected the entire capture and updated_at should be the only volatile field
There was a problem hiding this comment.
Sounds good. It's possible there are other volatile fields but they aren't as obviously named as updated_at, so I wouldn't spend a ton of time trying to figure those out proactively. We'll catch those if/when those fields are updated and the capture snapshot test fails, then we'll redact them. For now, I wouldn't suggest putting in any more effort beyond redacting those volatile fields we can easily identify, like updated_at.
| total_count += 1 | ||
| yield child | ||
|
|
||
| log.info(f"Fetched {total_count} event_invitees in window") |
There was a problem hiding this comment.
nit: it could be helpful to include the window's bounds here.
| log.info(f"Fetched {total_count} event_invitees in window") | |
| log.info(f"Fetched {total_count} event_invitees in window", { | |
| "params": params, | |
| }) |
| doc_count += 1 | ||
| yield item | ||
|
|
||
| log.info(f"Fetched {doc_count} scheduled_events in window") |
There was a problem hiding this comment.
nit: similar to this comment, it could be helpful for debugging purposes to include the window's bounds in this log.
| task, | ||
| fetch_changes=functools.partial(fetch_entity, entity_cls, org_uri, http), | ||
| fetch_page=functools.partial( | ||
| backfill_entity, entity_cls, org_uri, http, config |
There was a problem hiding this comment.
Instead of binding the config that contains the unencrypted credentials, can we pass in only the fields from the config that are necessary? That would minimize the opportunity for the unencrypted credentials to accidently get included in an info level log in the backfill_entity or other fetch functions via the config.
| binding_index, | ||
| state, | ||
| task, | ||
| fetch_changes=functools.partial(fetch_fn, org_uri, http, config), |
There was a problem hiding this comment.
Same comment here about passing in only the config fields we need instead of binding the config to the fetch_changes function.
Description:
Adds a new connector for the Calendly scheduling tool.
Workflow steps:
(How does one use this feature, and how has it changed)
Documentation links affected:
(list any documentation links that you created, or existing ones that you've identified as needing updates, along with a brief description)
Notes for reviewers:
Connector has been tested locally though
flowctl previewand a local deployment.