ci: test connector images against an SDM base image built from the current branch - #1085
ci: test connector images against an SDM base image built from the current branch#1085Daryna Ishchenko (darynaishchenko) wants to merge 6 commits into
Conversation
🤖 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 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@devin/1785245034-ci-image-test-branch-cdk#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 devin/1785245034-ci-image-test-branch-cdkPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
…ranch Co-Authored-By: Daryna Ishchenko <darina.ishchenko17@gmail.com>
… it as base Co-Authored-By: Daryna Ishchenko <darina.ishchenko17@gmail.com>
fc4af19 to
909c48b
Compare
📝 WalkthroughWalkthroughThe change adds optional base-image overrides to Docker image builds, CLI commands, and standard tests. Docker check tests now validate emitted connection statuses, and CI builds and uses a local declarative-manifest image for manifest-only connector tests. ChangesCustom Base Image Testing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant AirbyteCDK
participant Docker
GitHubActions->>Docker: Build airbyte/source-declarative-manifest:dev
GitHubActions->>AirbyteCDK: Run image test with --base-image
AirbyteCDK->>Docker: Build connector image using the local base image
AirbyteCDK->>Docker: Run connector check
Docker-->>AirbyteCDK: Return connection status messages
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/connector-tests.yml:
- Around line 228-231: Update the “Checkout CDK” actions/checkout step to set
persist-credentials to false, ensuring the checkout token is not retained in Git
configuration while preserving the existing full-history checkout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e4c8ffd-b176-4751-a11b-724cd5b0157a
📒 Files selected for processing (6)
.github/workflows/connector-tests.ymlairbyte_cdk/cli/airbyte_cdk/_image.pyairbyte_cdk/test/standard_tests/docker_base.pyairbyte_cdk/test/standard_tests/pytest_hooks.pyairbyte_cdk/utils/docker.pyunit_tests/utils/test_docker_build.py
…nce in new job Co-Authored-By: Daryna Ishchenko <darina.ishchenko17@gmail.com>
PyTest Results (Full)4 144 tests 4 132 ✅ 9m 18s ⏱️ Results for commit e4dbd33. ♻️ This comment has been updated with latest results. |
…ch-built SDM Co-Authored-By: Daryna Ishchenko <darina.ishchenko17@gmail.com>
Co-Authored-By: Daryna Ishchenko <darina.ishchenko17@gmail.com>
…mage-test-branch-cdk
Summary
Closes the third CI gap that let #1076 ship the #1082 regression without any check going red: the Docker image tests always built connector images
FROMthe already-published SDM base image frommetadata.yaml, so the PR branch's CDK was never inside the container. A second gap compounded it — a failing in-containercheckexits 0 and only reportsCONNECTION_STATUS: FAILED, which the harness never asserted on.Three changes:
Base-image override, plumbed end to end — build a connector image
FROMan arbitrary (e.g. locally built, unpublished) SDM image:base_image_override or metadata.data.connectorBuildOptions.baseImage— metadata remains the default when the flag is absent.The existing "Test Connectors" matrix now tests the branch's CDK inside the container. For manifest-only connectors (source-intercom, source-pokeapi), the matrix job builds
airbyte/source-declarative-manifest:devfrom the current branch and runsimage test --base-imagewith it — previously those container tests ran the already-released CDK from the published base image, so they couldn't catch a CDK regression pre-release. Python connectors already get the branch CDK in their images: theBump to Dev Branch CDKstep pins the connector's CDK dependency to the branch ref, which the container image'spoetry installthen picks up.test_docker_image_build_and_checknow asserts the check outcome — previously acheckreturningCONNECTION_STATUS: FAILED(exit code 0, no trace error) passed the test. Now the lastconnectionStatusmust beStatus.SUCCEEDED(exception-expecting scenarios are already skipped earlier), so a failing in-containercheckactually fails the test.Verified locally with the exact CI command:
7.23.7(the fix(declarative): requireAIRBYTE_ENABLE_UNSAFE_CODEto instantiateCustom*components from YAML #1076 CDK):test_docker_image_build_and_checkfails — check reportsCustom connector code is not permitted in this environment.Also adds a parametrized unit test asserting
base_image_overrideis threaded into theBASE_IMAGEbuild arg (and that metadata remains the default).CI state
While #1083 was unmerged, the matrix container test for
source-intercom(bundledcomponents.py) correctly failed on this PR withCustom connector code is not permitted in this environment— proving the guard catches the #1082 regression when the branch-built SDM carries it. Now that #1083 is merged and this branch is updated withmain, that check passes.Note: the new
check-outcome assertion also surfaced thatdestination-motherduck's in-containercheckhas been silently failing in CI (MotherDuck token auth error + aPermissionError), previously masked because the harness ignoredCONNECTION_STATUS: FAILED. That looks like a connector-secrets/environment issue, independent of this PR's code.Summary by CodeRabbit
New Features
Tests