chore: use sdm cli as dockerfile entrypoint #470
Conversation
…into christo/sdm-cli-dockerfile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThis update modifies the Dockerfile to change how the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DockerContainer
participant source-declarative-manifest CLI
User->>DockerContainer: Start container
DockerContainer->>source-declarative-manifest CLI: Invoke CLI entrypoint
source-declarative-manifest CLI->>source-declarative-manifest CLI: Execute command (spec/check/discover/read)
Possibly related PRs
Suggested labels
Suggested reviewers
Would you like to further clarify the README with additional troubleshooting steps for other common environments, or is the current guidance sufficient, wdyt? 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:
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.
Actionable comments posted: 1
🧹 Nitpick comments (6)
Dockerfile (2)
20-22: Optimize pip install to reduce image size?Right now we run:
RUN pip install dist/*.whlWould you consider adding
--no-cache-dirto prevent wheel caching and keep the image slimmer, e.g.:RUN pip install --no-cache-dir dist/*.whlwdyt?
36-38: Remove or leverage the AIRBYTE_ENTRYPOINT env var?I see we set:
ENV AIRBYTE_ENTRYPOINT="source-declarative-manifest" ENTRYPOINT ["source-declarative-manifest"]Should we either drop the unused
AIRBYTE_ENTRYPOINTvariable or switch to a shell form entrypoint that references it, so it’s single-source-of-truth? wdyt?airbyte_cdk/cli/README.md (4)
5-17: Add an offline YAML→JSON option?You suggest an online tool for converting manifests, but many devs prefer local tooling. Could we mention a
yqexample, e.g.:yq eval -o=json manifest.yaml > manifest.jsonwdyt?
19-32: Expand examples for all commands?Listing
spec,check,discover, andreadis helpful. Could we include short CLI snippets forspecanddiscoverhere as well, so users see full usage at a glance? wdyt?
38-44: Clarify step sequencing and placeholdersStep 1 covers conversion nicely. Maybe call out that the JSON must be valid (no comments) and note the placeholder will need removal. Also reiterate the
yqalternative here? wdyt?
57-63: Include localspecanddiscoverexamples too?You show
checkandreadfor local tests—nice! Could we also demonstratespecanddiscoverin the same section, so the guide covers all CLI commands? wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Dockerfile(2 hunks)airbyte_cdk/cli/README.md(1 hunks)
🔇 Additional comments (2)
airbyte_cdk/cli/README.md (2)
1-4: Great CLI introductionThe title and opening lines clearly explain the purpose of this guide—nice work!
34-37: Local dev caution looks solidThe "remote manifest mode" note is clear and very useful for people hacking locally.
| Example: | ||
|
|
||
| ```json | ||
| { | ||
| "api_key": "very_secret_key", | ||
| "start_time": "04:20", | ||
| "__injected_declarative_manifest": { | ||
| // Insert the JSON version of your manifest here | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
JSON snippet contains invalid comments
JSON doesn’t support // comments, which will break parsing. Should we replace the inline comment with a placeholder or remove it entirely? For example:
- "__injected_declarative_manifest": {
- // Insert the JSON version of your manifest here
- }
+ "__injected_declarative_manifest": { /* insert your manifest JSON here */ }wdyt?
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@coderabbitai Oh nice! I wasn't aware of that placeholder pattern, please go ahead and make this change.
There was a problem hiding this comment.
@ChristoGrab Great! I'll plan that change for you now. The JSON comment will be updated to use the proper format:
%%{init: {"securityLevel":"loose"}}%%
flowchart TB
A["Fix JSON comment in README"] --> B["Replace // style comment with /* */ style"]
Another agent will execute this change for you shortly. This will ensure that the example JSON in the documentation is properly formatted and won't cause parsing errors when copied by users.
⏳ Preparing changes...
There was a problem hiding this comment.
A summary of the edits CodeRabbit can apply:
We will replace the invalid
//comment with a proper/* */comment in the JSON example in airbyte_cdk/cli/README.md.
A plan of each step that CodeRabbit will take in order to execute the edits for each file (with inline rough draft snippets):
✏️ airbyte_cdk/cli/README.md
Delete lines 51–53 (the opening brace, the // comment line, and the closing brace) and insert a single line:
"__injected_declarative_manifest": { /* insert your manifest JSON here */ }Ensure there are two spaces of indentation before "__injected_declarative_manifest" to match the surrounding JSON block, and verify no trailing commas or extra braces are introduced.
Save the file and preview the Markdown rendering to confirm that the JSON block is syntactically valid and that the comment displays correctly in all rendered views.
A summary of the context that CodeRabbit is considering across the codebase:
Locate the JSON snippet containing the invalid
//comment in airbyte_cdk/cli/README.md to confirm the exact line numbers to modify.
-
✅ Create PR with these edits
-
📋 Get copyable edits
-
📌 Create commit in current branch
-
🔄 Try a different approach
-
⏪ Go back to ordinary chat
There was a problem hiding this comment.
This looks great, @ChristoGrab . One suggestion regarding docs, non-blocking.
There was a problem hiding this comment.
@ChristoGrab - If we move this markdown content into the cli/__init__.py docstring, it will automatically render in pdoc when we generate the API docs via GitHub pages. Wdyt?
There was a problem hiding this comment.
I think there's also a syntax to import markdown file into the pdoc generation if you want to keep it a separate file.
What
Small updates to the python CDK's dockerfile to use the source-declarative-manifest cli as the entrypoint rather than running the code via python call.
Resolves #11698
Summary by CodeRabbit
Documentation
Chores