fix(declarative): [DO-NOT-MERGE] pass config into declarative source - #1113
fix(declarative): [DO-NOT-MERGE] pass config into declarative source#1113Aaron ("AJ") Steers (aaronsteers) wants to merge 13 commits into
Conversation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
🤖 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. 💡 Show Tips and TricksTesting 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/1787019774-declarative-config-passthrough' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1787019774-declarative-config-passthrough'PR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
📚 Show Repo GuidanceHelpful ResourcesCommunity SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
Co-Authored-By: AJ Steers <aj@airbyte.io>
Co-Authored-By: AJ Steers <aj@airbyte.io>
Co-Authored-By: AJ Steers <aj@airbyte.io>
Co-Authored-By: AJ Steers <aj@airbyte.io>
Co-Authored-By: AJ Steers <aj@airbyte.io>
📝 WalkthroughWalkthroughThe change passes source configuration from ChangesDeclarative configuration propagation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🟡 Moderate · up to The change forwards connector configuration into declarative execution, but optional configuration can still reach component injection as None and shared defaults can leak injected data between runs; this should be corrected before merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-Authored-By: AJ Steers <aj@airbyte.io>
Co-Authored-By: AJ Steers <aj@airbyte.io>
Co-Authored-By: AJ Steers <aj@airbyte.io>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Fixes declarative manifest interpolation by ensuring connector configuration is available to ConcurrentDeclarativeSource during manifest resolution, including config supplied at construction time and config supplied later via --config (e.g., after source.set_config()).
Changes:
- Threads
configfromget_source()throughget_connector_executor()intoDeclarativeExecutor. - Updates
DeclarativeExecutor.execute()to read the--configfile at runtime and merge it with constructor config when creating the declarative source. - Adds unit tests validating config propagation, late config handling, injected component key preservation, and caller-dict immutability.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/unit_tests/test_declarative_executor.py | Adds unit tests covering declarative executor config resolution and immutability. |
| airbyte/sources/util.py | Passes config from get_source() into the connector executor factory. |
| airbyte/_executors/util.py | Extends get_connector_executor() to accept config and forward it to DeclarativeExecutor. |
| airbyte/_executors/declarative.py | Accepts config in DeclarativeExecutor, reads --config at execute time, and recreates the declarative source with merged config. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| - If `manifest` is a path, it will be read as a json file. | ||
| - If `manifest` is a string, it will be parsed as an HTTP path. | ||
| - If `manifest` is a dict, it will be used as is. | ||
| - If `config` is provided, it will be used to resolve manifest interpolations. | ||
| - If `components_py` is provided, components will be injected into the source. | ||
| - If `components_py_checksum` is not provided, it will be calculated automatically. |
There was a problem hiding this comment.
🙋 Not changing here, flagging for the maintainer. Correct that those two bullets are wrong — manifest is parsed with yaml.safe_load() and the annotation is dict | Path, so neither "read as a json file" nor the str/HTTP-path bullet matches the code. Both predate this PR, though; this branch only added the config bullet. Leaving them alone to keep this diff to the #868 fix — happy to correct them here if Aaron ("AJ") Steers (@aaronsteers) prefers.
| mapped_args: list[str] = self.map_cli_args(args) | ||
| args_config = _get_config_from_args(mapped_args) | ||
| source_entrypoint = AirbyteEntrypoint( | ||
| self._create_declarative_source( | ||
| {**self._config_dict, **args_config}, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
👍 On it. Agreed — the injected keys are the executor's own and shouldn't be clobberable by whatever lands in the --config file. Pushing a follow-up commit so user config still wins for normal fields while __injected_components_py* is re-applied after the merge, with a regression test that a bogus injected value in the config file is ignored.
Co-Authored-By: AJ Steers <aj@airbyte.io>
Code Coverage OverviewLanguages: Python Python / code-coverage/pytest-fastThe overall coverage in commit 95d7aae in the Show a code coverage summary of the most impacted files.
Python / code-coverage/pytest-no-credsThe overall coverage in commit 95d7aae in the Show a code coverage summary of the most impacted files.
Python / code-coverage/pytestThe overall coverage in commit 95d7aae in the Show a code coverage summary of the most impacted files.
Updated |
devin-ai-integration
Bot
changed the title
devin-ai-integration
Bot
changed the title
Replaces: #869
Summary
This PR extends the contribution from Adileo Barone (@adileo) (thank you! 🙏), cherry-picked from #869 so the original commit authorship is preserved. Requested by Aaron ("AJ") Steers (@aaronsteers).
Closes #868.
Declarative sources were built with an effectively empty config, so every
{{ config[...] }}interpolation a manifest performs at resolution time (dynamic streams, stream templates) resolved to nothing:The contributor's commits thread
configfromget_source()→get_connector_executor()→DeclarativeExecutor.__init__. The maintainer follow-ups then cover the paths that construction-time config alone misses — config supplied later viasource.set_config(), andsecret_reference::values, which are only hydrated into the--configtemp file at execute time:Constructor config stays as the fallback, and the merge order keeps the injected
__injected_components_pykeys intact. A missing, malformed or non-dict--configfile falls back to the constructor config rather than raising.Also in the follow-ups:
configno longer defaults to a mutable{}(ruff B006) and is copied into the executor, so component injection can't mutate the caller's dict; parameters aftermanifestare now keyword-only, soconfigcan't be silently bound positionally wherecomponents_pyused to sit.Test plan
New offline unit tests in
tests/unit_tests/test_declarative_executor.pycover config viaget_source(), config viaset_config()after construction, survival of the injected components keys, and caller-dict immutability. All three fail with the fix reverted.Manual repro of #868 against a local HTTP server, using a manifest whose
url_baseis{{ config['base_url'] }}, so a resolved config is observable as a real request rather than inferred:source.check()main(f65f227)get_source()MissingSchema)main(f65f227)set_config()MissingSchema)get_source()/itemsset_config()/itemsLocal checks:
ruff check .,ruff format --check .,mypy airbyte,pytest tests/unit_tests/test_declarative_executor.py,pytest tests/integration_tests/destinations/test_source_to_destination.py— all pass. Credential-gated suites were not run locally.Link to Devin session: https://app.devin.ai/sessions/e76350e6c7c043c791504ee50fc27bbe