Conversation
🧹 Python Code Quality Check📎 Download full report from workflow artifacts. 📌 Only Python files changed in this PR were checked. This comment is auto-updated with every commit. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new Zeenea connector that appears to use a decorator-based API pattern for the Fivetran Connector SDK. However, this implementation deviates significantly from the established SDK patterns used throughout the repository and violates multiple critical SDK requirements.
Key Changes:
- Adds a new connector for the Zeenea data catalog API
- Attempts to use decorator-based configuration (
@connector,config.Config,schema.Schema) - Implements data fetching from Zeenea API endpoint
| """ | ||
|
|
||
| import requests | ||
| from fivetran_connector_sdk import connector, config, state, records, log, schema |
There was a problem hiding this comment.
This import statement uses an API pattern that does not exist in the Fivetran Connector SDK. The correct imports should be: from fivetran_connector_sdk import Connector for the connector class, from fivetran_connector_sdk import Operations as op for data operations, and from fivetran_connector_sdk import Logging as log for logging. The decorator-based pattern with @connector, config.Config, state.Context, records.write, and schema.Schema is not part of the standard SDK API.
| Fetches data catalog metadata from Zeenea API. | ||
| """ | ||
|
|
||
| import requests |
There was a problem hiding this comment.
Missing required comment explaining the purpose of this import. All imports must include inline comments explaining why they are needed. Example: import requests # For making HTTP API requests to Zeenea API
| CONFIG = config.Config( | ||
| base_url=config.StringField(description="Zeenea API base URL"), | ||
| api_token=config.SecretField(description="Zeenea API token") | ||
| ) |
There was a problem hiding this comment.
This declarative configuration pattern is not part of the Fivetran Connector SDK. Configuration should be provided via a configuration.json file, and the SDK automatically validates required fields. Remove this CONFIG object entirely. If configuration validation is needed, it should only be done in the schema() function for schema generation purposes.
| SCHEMA = schema.Schema( | ||
| name="zeenea_assets", | ||
| columns={ | ||
| "id": schema.StringColumn(), | ||
| "name": schema.StringColumn(), | ||
| "type": schema.StringColumn(), | ||
| "updatedAt": schema.StringColumn(), | ||
| "metadata": schema.JSONColumn(), | ||
| } | ||
| ) |
There was a problem hiding this comment.
This declarative schema pattern is not part of the Fivetran Connector SDK. Schema should be defined using a schema(configuration: dict) function that returns a list of table dictionaries. Replace this with a proper schema function following the SDK pattern. Example: def schema(configuration: dict): return [{\"table\": \"zeenea_assets\", \"primary_key\": [\"id\"]}]
| @connector( | ||
| name="ZeeneaConnector", | ||
| version="0.1.0", | ||
| config=CONFIG, | ||
| schema=SCHEMA, | ||
| ) | ||
| def run_connector(ctx: state.Context): |
There was a problem hiding this comment.
The decorator-based @connector pattern is not part of the Fivetran Connector SDK. The SDK requires an update(configuration: dict, state: dict) function and initialization with connector = Connector(update=update, schema=schema). Remove this decorator and implement the standard update() function pattern.
| """ | ||
| Zeenea connector for Fivetran Connector SDK. | ||
| Fetches data catalog metadata from Zeenea API. | ||
| """ |
There was a problem hiding this comment.
Missing required connector initialization and main block at the end of the file. After all functions, add: connector = Connector(update=update, schema=schema) followed by the standard main block for debugging with if __name__ == \"__main__\": that includes configuration loading and connector.debug(configuration=configuration). See template_example_connector/connector.py for the required comments and structure.
| config=CONFIG, | ||
| schema=SCHEMA, | ||
| ) | ||
| def run_connector(ctx: state.Context): |
There was a problem hiding this comment.
Missing required update() function. The SDK requires a function named update(configuration: dict, state: dict) as the entry point for data syncing. This function must include the required docstring: Define the update function which lets you configure how your connector fetches data. See the technical reference documentation for more details on the update function: https://fivetran.com/docs/connectors/connector-sdk/technical-reference#update with Args section documenting both parameters.
| SCHEMA = schema.Schema( | ||
| name="zeenea_assets", | ||
| columns={ | ||
| "id": schema.StringColumn(), |
There was a problem hiding this comment.
Missing primary key definition in schema. The table 'zeenea_assets' appears to have an 'id' column that should likely be the primary key. When implementing the proper schema() function, ensure to include \"primary_key\": [\"id\"] in the table definition to enable proper upsert behavior.
| response = requests.get(f"{ctx.config.base_url}/api/assets", headers=headers) | ||
| response.raise_for_status() |
There was a problem hiding this comment.
Using bare raise_for_status() without specific exception handling. Catch specific exceptions and distinguish between retryable errors (network issues, 5xx errors) and permanent failures (401, 403, 400). Implement proper error handling: try: ... except (requests.HTTPError, requests.Timeout, requests.ConnectionError) as e: and log errors using log.severe(f\"API request failed: {str(e)}\").
| """ | ||
|
|
||
| import requests | ||
| from fivetran_connector_sdk import connector, config, state, records, log, schema |
There was a problem hiding this comment.
Import of 'log' is not used.
| from fivetran_connector_sdk import connector, config, state, records, log, schema | |
| from fivetran_connector_sdk import connector, config, state, records, schema |
fivetran-rishabhghosh
left a comment
There was a problem hiding this comment.
Please address copilot comments and add description
| response = requests.get(f"{ctx.config.base_url}/api/assets", headers=headers) | ||
| response.raise_for_status() |
There was a problem hiding this comment.
The connector is missing retry logic with exponential backoff for the API request. API calls can fail due to transient errors (network issues, rate limits, 5xx errors) and should be retried.
Required implementation pattern:
__MAX_RETRIES = 3
for attempt in range(__MAX_RETRIES):
try:
response = requests.get(f"{base_url}/api/assets", headers=headers)
response.raise_for_status()
break
except (requests.Timeout, requests.ConnectionError) as e:
if attempt == __MAX_RETRIES - 1:
raise
sleep_time = min(60, 2 ** attempt)
log.warning(f"Retry {attempt + 1}/{__MAX_RETRIES} after {sleep_time}s")
time.sleep(sleep_time)There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (2)
connectors/zeenea/connector.py:23
- The schema.Schema decorator pattern is not part of the Fivetran Connector SDK. You must define a schema() function that takes a configuration dictionary parameter and returns a list of table schema dictionaries. The function should follow the format shown in the template with table name, primary_key, and optionally columns with data types.
SCHEMA = schema.Schema(
name="zeenea_assets",
columns={
"id": schema.StringColumn(),
"name": schema.StringColumn(),
"type": schema.StringColumn(),
"updatedAt": schema.StringColumn(),
"metadata": schema.JSONColumn(),
}
)
connectors/zeenea/connector.py:4
- Missing required validate_configuration() function. This function is mandatory and must be defined to validate all required configuration parameters (base_url, api_token) before use. It should check for presence and optionally validate formats (e.g., valid URL format). The function must be called at the start of the update() function.
"""
Zeenea connector for Fivetran Connector SDK.
Fetches data catalog metadata from Zeenea API.
"""
| @@ -0,0 +1,39 @@ | |||
| """ | |||
There was a problem hiding this comment.
This connector is missing the required configuration.json file. Every connector must have a configuration.json file that defines the configuration parameters with placeholder values (e.g., "base_url": "<YOUR_ZEENEA_BASE_URL>", "api_token": "<YOUR_API_TOKEN>"). The file must be created in the same directory as connector.py.
| response = requests.get(f"{ctx.config.base_url}/api/assets", headers=headers) | ||
| response.raise_for_status() | ||
|
|
||
| for asset in response.json().get("items", []): |
There was a problem hiding this comment.
This code loads all data into memory at once by calling response.json().get("items", []) and then iterating over it. This violates memory management best practices and will cause memory overflow for large datasets. You must implement pagination to process data in batches. The API likely supports pagination parameters (e.g., page, limit, offset, or cursor-based pagination). Process each page immediately and checkpoint after each page.
| """ | ||
| Zeenea connector for Fivetran Connector SDK. | ||
| Fetches data catalog metadata from Zeenea API. | ||
| """ | ||
|
|
||
| import requests | ||
| from fivetran_connector_sdk import connector, config, state, records, log, schema | ||
|
|
||
| CONFIG = config.Config( | ||
| base_url=config.StringField(description="Zeenea API base URL"), | ||
| api_token=config.SecretField(description="Zeenea API token") | ||
| ) | ||
|
|
||
| SCHEMA = schema.Schema( | ||
| name="zeenea_assets", | ||
| columns={ | ||
| "id": schema.StringColumn(), | ||
| "name": schema.StringColumn(), | ||
| "type": schema.StringColumn(), | ||
| "updatedAt": schema.StringColumn(), | ||
| "metadata": schema.JSONColumn(), | ||
| } | ||
| ) | ||
|
|
||
| @connector( | ||
| name="ZeeneaConnector", | ||
| version="0.1.0", | ||
| config=CONFIG, | ||
| schema=SCHEMA, | ||
| ) | ||
| def run_connector(ctx: state.Context): | ||
| headers = {"Authorization": f"Bearer {ctx.config.api_token}"} | ||
| response = requests.get(f"{ctx.config.base_url}/api/assets", headers=headers) | ||
| response.raise_for_status() | ||
|
|
||
| for asset in response.json().get("items", []): | ||
| records.write("zeenea_assets", asset) | ||
|
|
||
| return ctx.update_state({"last_sync": "now"}) |
There was a problem hiding this comment.
Missing the required first log statement. The update() function must start with log.warning("Example: <CATEGORY> : <EXAMPLE_NAME>") where CATEGORY should be something like "API_CONNECTOR" or "DATA_CATALOG" and EXAMPLE_NAME should be "ZEENEA".
| SCHEMA = schema.Schema( | ||
| name="zeenea_assets", | ||
| columns={ | ||
| "id": schema.StringColumn(), | ||
| "name": schema.StringColumn(), | ||
| "type": schema.StringColumn(), | ||
| "updatedAt": schema.StringColumn(), | ||
| "metadata": schema.JSONColumn(), | ||
| } | ||
| ) |
There was a problem hiding this comment.
The schema definition is missing the primary_key field. You must define primary_key to ensure proper data deduplication and updates. The "id" field should be specified as the primary key: "primary_key": ["id"]
| @@ -0,0 +1,39 @@ | |||
| """ | |||
There was a problem hiding this comment.
This connector is missing the required README.md file. Every connector must have a comprehensive README.md following the template structure with sections for: Connector overview, Requirements, Getting started, Features, Data handling, Error handling, Tables created, and Additional considerations. The README must have a single H1 heading in the format # Zeenea Connector Example.
| headers = {"Authorization": f"Bearer {ctx.config.api_token}"} | ||
| response = requests.get(f"{ctx.config.base_url}/api/assets", headers=headers) |
There was a problem hiding this comment.
Configuration access pattern is incorrect. In SDK v2+, configuration should be accessed as a dictionary (e.g., configuration["api_token"] and configuration["base_url"]), not as object attributes. The ctx.config pattern shown here is not part of the SDK.
|
This PR is not structured as per the CSDK Examples guidelines. It only has a connector file. There are multiple formatting and logical issues in the connector. The review comments are added. |
|
Surabhi Singh seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Jira ticket
Closes
<ADD TICKET LINK HERE, EACH PR MUST BE LINKED TO A JIRA TICKET>Description of Change
<MENTION A SHORT DESCRIPTION OF YOUR CHANGES HERE>Testing
<MENTION ABOUT YOUR TESTING DETAILS HERE, ATTACH SCREENSHOTS IF NEEDED (WITHOUT PII)>Checklist
Some tips and links to help validate your PR:
fivetran debugcommand.