-
Notifications
You must be signed in to change notification settings - Fork 30
fix: add interpolation_context to auth fields #659
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
fix: add interpolation_context to auth fields #659
Conversation
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Testing This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@lmossman/add-interpolation-context-to-auth-fields#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch lmossman/add-interpolation-context-to-auth-fields Helpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
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.
Pull Request Overview
This PR fixes missing interpolation_context
properties in authentication field definitions within the declarative component schema. The change enables the Builder UI to properly render JinjaInput components for these fields, which provide suggestions and formatting for Jinja expressions that reference configuration variables.
Key Changes
- Added
interpolation_context: [config]
to JWT authentication secret key field - Added
interpolation_context: [config]
to OAuth2 client ID, client secret, refresh token, and access token fields
airbyte_cdk/sources/declarative/declarative_component_schema.yaml
Outdated
Show resolved
Hide resolved
airbyte_cdk/sources/declarative/declarative_component_schema.yaml
Outdated
Show resolved
Hide resolved
📝 WalkthroughWalkthroughThe declarative component schema YAML file was updated to add an Changes
Suggested labels
Suggested reviewers
Would you like to highlight these schema clarifications and example fixes in the changelog for connector developers, wdyt? Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (4)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
1224-1285
: Consistent update across OAuth fields – consider adding the same fortoken_refresh_endpoint
?
client_id
,client_secret
,refresh_token
, andaccess_token_value
now advertise["config"]
as their interpolation context – this aligns them with other auth components 💯.
For full parity, users sometimes template thetoken_refresh_endpoint
itself (e.g., multi-tenant hosts). Would adding aninterpolation_context
there make sense too, or is that intentionally static? wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml
(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: pnilan
PR: airbytehq/airbyte-python-cdk#0
File: :0-0
Timestamp: 2024-12-11T16:34:46.319Z
Learning: In the airbytehq/airbyte-python-cdk repository, the `declarative_component_schema.py` file is auto-generated from `declarative_component_schema.yaml` and should be ignored in the recommended reviewing order.
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (3)
undefined
<retrieved_learning>
Learnt from: pnilan
PR: airbytehq/airbyte-python-cdk#0
File: :0-0
Timestamp: 2024-12-11T16:34:46.319Z
Learning: In the airbytehq/airbyte-python-cdk repository, the declarative_component_schema.py
file is auto-generated from declarative_component_schema.yaml
and should be ignored in the recommended reviewing order.
</retrieved_learning>
<retrieved_learning>
Learnt from: ChristoGrab
PR: #58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the YamlDeclarativeSource
class in airbyte_cdk/sources/declarative/yaml_declarative_source.py
, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
</retrieved_learning>
<retrieved_learning>
Learnt from: aaronsteers
PR: #58
File: airbyte_cdk/cli/source_declarative_manifest/_run.py:62-65
Timestamp: 2024-11-15T01:04:21.272Z
Learning: The files in airbyte_cdk/cli/source_declarative_manifest/
, including _run.py
, are imported from another repository, and changes to these files should be minimized or avoided when possible to maintain consistency.
</retrieved_learning>
⏰ 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). (5)
- GitHub Check: Check: source-shopify
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (1)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
1088-1102
: 👍 Interpolation context forsecret_key
looks correct – did we remember to regenerate the autogen.py
schema afterwards?Adding
interpolation_context: ["config"]
will unlock the JinjaInput in Builder – nice.
Becausedeclarative_component_schema.py
is generated from this YAML, a quick follow-up check that the file was (or will be) re-generated keeps tooling in sync, wdyt?
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
PyTest Results (Full)3 698 tests 3 687 ✅ 17m 58s ⏱️ Results for commit e264808. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
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.
Nice catch @lmossman!
interpolation_context
was missing from multiple auth fields that commonly need to access theconfig
interpolation variable.This is a problem for the Builder UI, as
interpolation_context
is how the Builder UI knows whether it should render the JinjaInput component for a field, which offers suggestions and formatting for jinja expressions.This PR fixes the issue by adding this property to the fields that were missing it.
Summary by CodeRabbit