-
Notifications
You must be signed in to change notification settings - Fork 6
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: ensure SDM image maintains airbyte-ci compatibility #90
Conversation
📝 WalkthroughWalkthroughThe changes in the Dockerfile involve modifications to the build process and entrypoint configuration. The structure of the integration code has been altered, with the removal of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 1
🧹 Outside diff range and nitpick comments (1)
Dockerfile (1)
16-28
: Overall architecture looks solid 🎯The approach of recreating the expected structure while maintaining a clean container image is clever. It solves the airbyte-ci compatibility issue while keeping things maintainable.
One architectural consideration: have you thought about maintaining these files in the repo instead of copying from the installed package? This could make updates more explicit and trackable. WDYT?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Dockerfile
(1 hunks)
🔇 Additional comments (2)
Dockerfile (2)
23-24
: LGTM! Good practice for image size optimization 👍
The cleanup of build artifacts helps keep the image slim. Nice work!
27-28
: Verify entrypoint compatibility with airbyte-ci
The entrypoint changes look aligned with the PR objectives. Just to be thorough, should we verify:
- That the path
/airbyte/integration_code/main.py
is consistent across the codebase? - That this matches exactly what airbyte-ci expects?
Let's check for path consistency and usage:
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.
Approved 👍
One comment to request we log an issue for follow-on, and I think Code Rabbit should take care of logging that.
What
When we implemented the new publish flow for SDM in Dockerhub, the built image diverged from the existing structure of source-declarative-manifest images built via the
base_images
package in our monorepo. This had the unintentional side effect of breaking existing airbyte-ci commands for manifest-only connectors, which have set expectations about the image's entrypoint and directory structure.Discrepancy
How
This PR modifies the built Dockerfile image to resemble the structure of the original SDM image by adding a
main.py
file at the project's root level and renaming_run.py
torun.py
within the built image.Testing Steps Used
6.7.0rc5
using the updated Dockerfile in this branch6.5.2
(stable monorepo release) and6.7.0rc5
from Dockerhubdocker run --rm -it --entrypoint /bin/bash source-declarative-manifest:<version>
for comparison6.7.0rc5
airbyte-ci connectors --name=source-pokeapi build
to verify the image builds locally6.7.0rc5
to verify the connector syncs on the platform