feat: Add capability to set testing values on custom connector projects in Builder UI#896
Conversation
…ts in Builder UI This adds the ability to set input values (testing values) when deploying custom YAML source definitions to the Connector Builder UI. This reduces friction by allowing users to immediately run test read operations after deploying a custom source. Changes: - Add update_connector_builder_project_testing_values() in api_util.py - Add set_testing_values() method to CustomCloudSourceDefinition - Add testing_values parameter to publish_custom_source_definition() Closes #895 Co-Authored-By: AJ Steers <aj@airbyte.io>
Original prompt from AJ Steers |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Testing This PyAirbyte VersionYou can test this version of PyAirbyte using the following: # Run PyAirbyte CLI from this branch:
uvx --from 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1765135139-add-testing-values-to-builder' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1765135139-add-testing-values-to-builder'Helpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
Community SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
📝 WalkthroughWalkthroughAdds support for persisting testing values for connector builder projects: new API helper, a setter on CustomCloudSourceDefinition, propagation of optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client (SDK/CLI)
participant Workspace as CloudWorkspace / MCP
participant Definition as CustomCloudSourceDefinition
participant APIUtil as api_util
participant Config as Config API
Client->>Workspace: publish_custom_source_definition(..., testing_values?)
Workspace->>Definition: construct from API response (YAML path)
alt testing_values provided
Definition->>Definition: ensure manifest/spec present (lazy load)
Definition->>APIUtil: update_connector_builder_project_testing_values(workspaceId, builderProjectId, testingValues, spec)
APIUtil->>Config: POST /connector_builder_projects/update_testing_values {workspaceId, builderProjectId, testingValues, spec}
Config-->>APIUtil: 200 OK / response
APIUtil-->>Definition: response dict
end
Workspace-->>Client: return CustomCloudSourceDefinition
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
airbyte/_util/api_util.py(1 hunks)airbyte/cloud/connectors.py(1 hunks)airbyte/cloud/workspaces.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
airbyte/cloud/connectors.py (2)
airbyte/exceptions.py (1)
PyAirbyteInputError(201-210)airbyte/_util/api_util.py (1)
update_connector_builder_project_testing_values(1567-1605)
airbyte/cloud/workspaces.py (1)
airbyte/cloud/connectors.py (3)
CustomCloudSourceDefinition(343-756)_from_yaml_response(628-640)set_testing_values(694-756)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (No Creds)
🔇 Additional comments (9)
airbyte/cloud/workspaces.py (3)
509-509: LGTM!The optional
testing_valuesparameter is well-placed and properly typed, enabling the one-shot publish + configure workflow described in the PR objectives.
523-525: LGTM!Clear documentation that explains the purpose and benefit of the
testing_valuesparameter.
590-598: LGTM!The implementation correctly constructs the
CustomCloudSourceDefinitionfrom the API response and conditionally applies testing values when provided. The method chaining pattern is clean and appropriate.airbyte/cloud/connectors.py (5)
694-714: LGTM!The method signature is clean, and the docstring provides comprehensive documentation about the purpose, parameters, return value, and exceptions.
715-719: LGTM!The guard clause correctly restricts this functionality to YAML definitions with a clear error message.
721-729: LGTM!The validation for
builder_project_idis appropriate with a clear error message and helpful context for debugging.
746-754: LGTM!The API call correctly passes all required parameters to the utility function with appropriate values from the workspace and method context.
756-756: LGTM!Returning
selfenables method chaining, which is consistent with other methods in this class.airbyte/_util/api_util.py (1)
1567-1606: Confirm the API payload format is accepted by the deprecated Config API endpoint.The spec construction in connectors.py (lines 735–750) can result in an empty dict when the manifest lacks a spec section. The
/connector_builder_projects/update_testing_valuesendpoint is part of the deprecated Config API with no published documentation. Consider verifying that the payload structure—particularly with an emptyspecfield—is accepted by the actual API, or document why this is acceptable for the deprecated endpoint.
PyTest Results (Full)416 tests ±0 399 ✅ - 1 23m 51s ⏱️ - 2m 28s For more details on these failures, see this check. Results for commit b13baa4. ± Comparison against base commit 69f6c96. ♻️ This comment has been updated with latest results. |
- Add testing_values and testing_values_secret_name parameters to publish_custom_source_definition MCP tool - Add testing_values and testing_values_secret_name parameters to update_custom_source_definition MCP tool - Make manifest_yaml optional in update_custom_source_definition (can now update just testing values) - Add guard statement to ensure at least one of manifest_yaml, testing_values, or testing_values_secret_name is provided - Use resolve_config() to handle secret resolution following existing PyAirbyte MCP conventions - Update docstrings to clarify that testing values replace the entire object (not merge) Co-Authored-By: AJ Steers <aj@airbyte.io>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
airbyte/cloud/connectors.py (1)
738-747: The fallback whenconnection_specificationis missing could include unwanted keys - would an empty dict be safer here?If
manifest_specdoesn't contain theconnection_specificationkey, the current fallback uses the entiremanifest_specobject as the value forconnectionSpecification. This could leak extra keys likedocumentationUrlinto the spec payload.Would it make sense to use
{}as the fallback instead, or raise an explicit error if the key is expected but missing? Something like:spec = { "connectionSpecification": manifest_spec.get( - "connection_specification", manifest_spec + "connection_specification", {} ), }What do you think?
🧹 Nitpick comments (1)
airbyte/mcp/cloud_ops.py (1)
1531-1556: Minor edge case: what happens iftesting_values_secret_nameis provided but resolves to an empty dict?The validation at line 1531 checks if the parameter is provided (not None), but after resolution,
testing_values_dictcould still beNone(line 1555-1556 withor None).If someone provides only
testing_values_secret_name="some_secret"and that secret resolves to{}:
- Validation passes (parameter was provided)
testing_values_dictbecomesNone- Neither manifest nor testing values are updated
- Success message is returned
This might be rare in practice, but could be confusing. Would it be worth adding a secondary check after resolution to warn or error if nothing will actually be updated? Something like:
if processed_manifest is None and testing_values_dict is None: raise PyAirbyteInputError( message="No changes to apply: manifest_yaml was not provided and testing_values resolved to empty.", ... )Totally optional - just flagging for awareness, wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
airbyte/_util/api_util.py(1 hunks)airbyte/cloud/connectors.py(1 hunks)airbyte/cloud/workspaces.py(3 hunks)airbyte/mcp/cloud_ops.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- airbyte/cloud/workspaces.py
- airbyte/_util/api_util.py
🧰 Additional context used
🧬 Code graph analysis (1)
airbyte/mcp/cloud_ops.py (4)
airbyte/mcp/_util.py (1)
resolve_config(129-226)airbyte/cloud/workspaces.py (3)
CloudWorkspace(64-664)publish_custom_source_definition(500-604)get_custom_source_definition(636-664)airbyte/cloud/connectors.py (5)
name(113-121)name(387-391)CustomCloudSourceDefinition(343-759)update_definition(521-588)set_testing_values(694-759)airbyte/exceptions.py (1)
PyAirbyteInputError(201-210)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (No Creds)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (5)
airbyte/cloud/connectors.py (1)
694-759: Nice addition - the method structure and error handling look good to me!The implementation follows the existing class patterns well:
- Proper YAML-only guard with clear error message
- Good contextual information in the
PyAirbyteInputError- Lazy loading of definition info
- Return self for chaining
One small observation: the spec construction at lines 739-747 only populates
connectionSpecificationwhenmanifest_specis truthy. If the manifest has nospecsection at all, an emptyspec={}is passed to the API. Is that the intended behavior, or should it raise an error in that case? Just want to confirm this is expected, wdyt?airbyte/mcp/cloud_ops.py (4)
1359-1383: Clear documentation on the new parameters!The descriptions nicely explain that testing values are replaced entirely (not merged), and that secret values take precedence. This should help users understand the behavior. Looks good to me!
1394-1411: LGTM!The resolution pattern and conditional check are clean. The
or Noneensures that an empty resolved dict doesn't trigger an unnecessary API call. Good approach!
1470-1479: Nice flexibility - making manifest_yaml optional is a good UX improvement!This allows users to update just testing values without having to re-provide the manifest. The updated description clearly explains this. 👍
1562-1578: Clean conditional logic!The flow handles both scenarios well:
- Manifest update only → new object from
update_definition- Testing values only → original definition object
- Both → testing values applied to the new object
The method chaining with
set_testing_valuesreturningselfworks nicely here.
Explicitly document that testing_values supports inline secret refs via 'secret_reference::ENV_VAR_NAME' syntax in both MCP tools. Co-Authored-By: AJ Steers <aj@airbyte.io>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
airbyte/mcp/cloud_ops.py (2)
1560-1573: Consider noting potential partial failure scenario in docstring?The manifest and testing values are updated via separate API calls. If the manifest update succeeds but
set_testing_valuesfails, the user ends up with a partially updated state. Since these are independent operations, this is probably acceptable behavior—but would it be helpful to mention this in the docstring so callers know these aren't applied atomically, wdyt?Otherwise, the logic looks correct—
custom_sourceis properly reassigned after the manifest update soset_testing_valuesoperates on the latest definition.
1549-1558: Optional: Consider extracting duplicated testing values resolutionThis resolution block is identical to lines 1395-1404 in
publish_custom_source_definition. If you anticipate adding this pattern to more functions, you could extract a small helper like_resolve_testing_values(testing_values, testing_values_secret_name). Just a thought for future maintainability—totally fine to leave as-is for now, wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte/mcp/cloud_ops.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
airbyte/mcp/cloud_ops.py (2)
airbyte/mcp/_util.py (1)
resolve_config(129-226)airbyte/exceptions.py (1)
PyAirbyteInputError(201-210)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (No Creds)
🔇 Additional comments (4)
airbyte/mcp/cloud_ops.py (4)
1359-1384: LGTM on the new parameter declarations!The new
testing_valuesandtesting_values_secret_nameparameters are well-typed and documented, following the established pattern ofconfig/config_secret_namepairs used elsewhere in this file. The description clearly communicates that values replace (not merge) existing testing values.
1395-1404: Minor edge case: empty dict becomesNoneThe
or Nonepattern means if a user passestesting_values={}, it becomesNoneand won't actually clear existing testing values. Is this the intended behavior? If users should be able to explicitly clear testing values by passing an empty dict, you might want to differentiate between "not provided" (None) and "explicitly empty" ({}), wdyt?That said, for a publish operation this is likely fine since there are no pre-existing values to clear.
1471-1480: Nice flexibility improvement!Making
manifest_yamloptional is a good UX enhancement—users can now update testing values without having to re-provide the manifest. The guard at line 1533 ensures at least one meaningful parameter is provided.
1530-1543: Good defensive validation!The guard clause clearly prevents no-op update calls, and the error message with context (definition_id, workspace_id) will help users understand what went wrong. Well done!
|
Devin, check the deptry issue or any other ci issues. |
|
Oh. Looks like that job just needs a retry |
|
Confirmed - the Deptry failure was a transient TLS connection error in GitHub Actions infrastructure ("Client network socket disconnected before secure TLS connection was established"). All other CI checks passed (19/20). A retry should resolve it. |
…r UI pattern Co-Authored-By: AJ Steers <aj@airbyte.io>
FastMCP was treating optional parameters as required when they had both Field(default=None) and = None at the end. Matching the pattern used by other optional parameters in the file (only Field(default=None)) fixes the issue. Co-Authored-By: AJ Steers <aj@airbyte.io>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte/mcp/cloud_ops.py (1)
1395-1405: Explicit empty testing_values ({}) are treated as “no update” — intentional?In both
publish_custom_source_definition(Lines 1395–1405) andupdate_custom_source_definition(Lines 1549–1558, 1572–1573),testing_values_dictis computed as:testing_values_dict = ( resolve_config( config=testing_values, config_secret_name=testing_values_secret_name, ) or None )Because of the
or None, an explicit empty object (e.g.,testing_values={}ortesting_values="{}") becomesNone, soset_testing_valuesis never called. That means there’s currently no way from the MCP side to intentionally “clear” previously‑stored testing values by setting them to{}; the call becomes a no‑op even though the docstrings say the entire testing values object is replaced by what’s provided.Would it be worth treating
{}as a valid, explicit update so that callers can clear testing values, by dropping theor Noneand letting{}flow through? For example:- testing_values_dict = ( - resolve_config( - config=testing_values, - config_secret_name=testing_values_secret_name, - ) - or None - ) + testing_values_dict = resolve_config( + config=testing_values, + config_secret_name=testing_values_secret_name, + )(Apply in both functions; the existing
if testing_values is not None or testing_values_secret_name is not None:guard already prevents accidental calls when nothing was supplied.) This would align behavior more closely with the “entire testing values object is overwritten” wording and still keep the API straightforward — wdyt?Also applies to: 1549-1558, 1572-1573
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte/mcp/cloud_ops.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
airbyte/mcp/cloud_ops.py (3)
airbyte/mcp/_util.py (1)
resolve_config(129-226)airbyte/cloud/connectors.py (4)
name(113-121)name(387-391)CustomCloudSourceDefinition(343-759)set_testing_values(694-759)airbyte/exceptions.py (1)
PyAirbyteInputError(201-210)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (No Creds)
🔇 Additional comments (1)
airbyte/mcp/cloud_ops.py (1)
1359-1384: Testing values parameters and wiring look consistent and well‑documentedThe addition of
testing_values/testing_values_secret_name(Lines 1359–1384) and the resolution viaresolve_configintotesting_values_dictpassed through toCloudWorkspace.publish_custom_source_definition(Line 1412) matches the patterns used elsewhere for config + secret handling and lines up withCustomCloudSourceDefinition.set_testing_valuessemantics inairbyte/cloud/connectors.py(full replacement, not merge). I don’t see any functional issues here and the docstrings clearly state the overwrite behavior and secret‑ref usage.Also applies to: 1395-1413
Aaron ("AJ") Steers (aaronsteers)
merged commit ff0f427
into
main
Aaron ("AJ") Steers (aaronsteers)
deleted the
devin/1765135139-add-testing-values-to-builder
branch
1 participant
feat: Add capability to set testing values on custom connector projects
Summary
This PR adds the ability to set input values (testing values) when deploying custom YAML source definitions to the Connector Builder UI. This addresses the friction described in #895 where users couldn't immediately run test read operations after deploying a custom source to Builder.
Changes:
update_connector_builder_project_testing_values()API function inapi_util.pythat calls the Config API endpoint/v1/connector_builder_projects/update_testing_valuesset_testing_values()method toCustomCloudSourceDefinitionfor setting testing values on an existing definitiontesting_valuesparameter toCloudWorkspace.publish_custom_source_definition()for one-shot publish + configureUpdates since last revision:
testing_valuesandtesting_values_secret_nameparameters to bothpublish_custom_source_definitionandupdate_custom_source_definitiontoolsmanifest_yamloptional inupdate_custom_source_definitionMCP tool (can now update just testing values)manifest_yaml,testing_values, ortesting_values_secret_nameresolve_config()for secret handling, following existing PyAirbyte MCP conventions (agents can reference secrets without passing raw values)documentationUrl,connectionSpecification, andadvancedAuthconnection_specification(now defaults to{}instead of the entire manifest spec)= Nonefrom MCP tool parameters (was causing validation errors treating optional params as required)Review & Testing Checklist for Human
testing_valuesparameter and verify the values appear in the Connector Builder UI and allow immediate test reads. This is the most important validation since the spec construction was derived from code analysis, not live testing.update_custom_source_definitionMCP tool with justtesting_values(nomanifest_yaml) to verify the code correctly fetches the manifest from the server to derive the spec.testing_values_secret_namecorrectly resolves secrets viaresolve_config()when used in MCP tools.Recommended test plan:
Notes
Closes #895
Link to Devin run: https://app.devin.ai/sessions/da20573387b04e8b84371218ef63bf1a
Requested by: AJ Steers (Aaron ("AJ") Steers (@aaronsteers))
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
Important
Auto-merge enabled.
This PR is set to merge automatically when all requirements are met.
Note
Auto-merge may have been disabled. Please check the PR status to confirm.