Add proxy and datasette autopull for latest tag#45
Conversation
📝 WalkthroughWalkthroughThe changes introduce conditional image update checks using a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Greptile SummaryThis PR introduces two features: (1) automatic update checks for Docker images tagged Key changes:
Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Start proxy/run/logs command] --> B{_is_latest_tag?}
B -- No --> E[Skip pull]
B -- Yes --> C[pull_if_newer]
C --> D{Image updated?}
D -- "logs.py ✅" --> F[Remove existing datasette container]
F --> G[ensure_datasette creates fresh container]
D -- "proxy.py / run.py ❌\nreturn value discarded" --> H[ensure_proxy called]
H --> I{Proxy already running?}
I -- Yes --> J[Return existing container\nNEW IMAGE IGNORED]
I -- No --> K[Start new proxy container]
E --> H
Last reviewed commit: 92243c3 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if _is_latest_tag(proxy_image): | ||
| info("Checking for proxy image updates…") | ||
| manager.pull_if_newer(proxy_image) |
There was a problem hiding this comment.
Proxy update not applied to running container
pull_if_newer returns True when a newer image was downloaded, but the result is discarded here. As a result, if the proxy container is already running, ensure_proxy will find it with status == "running" and return immediately without restarting it with the new image — so the update never takes effect.
Compare this with how logs.py handles the datasette update (lines 63–70): it checks the return value of pull_if_newer, and if an update was found, it explicitly removes the existing container before calling ensure_datasette, which then creates a fresh one.
The fix is to apply the same pattern:
if _is_latest_tag(proxy_image):
info("Checking for proxy image updates…")
updated = manager.pull_if_newer(proxy_image)
if updated:
info("New image available — restarting proxy")
existing = manager.find_proxy()
if existing:
existing.remove(force=True)| if _is_latest_tag(proxy_image): | ||
| manager.pull_if_newer(proxy_image) |
There was a problem hiding this comment.
Proxy image update silently ignored for running containers
Same issue as in proxy.py: the return value of pull_if_newer is discarded. If the proxy container is already running (e.g., started by a prior vp proxy start or a previous vp run), ensure_proxy returns the existing container unchanged, and the freshly-pulled image is never used.
There's also no informational log here telling the user that an update check is happening, unlike the equivalent code in proxy.py and logs.py.
if _is_latest_tag(proxy_image):
info("Checking for proxy image updates…")
updated = manager.pull_if_newer(proxy_image)
if updated:
info("New proxy image available — restarting proxy")
existing = manager.find_proxy()
if existing:
existing.remove(force=True)| def _is_latest_tag(image: str) -> bool: | ||
| """Return True when *image* uses the ``latest`` tag (explicitly or by omission).""" | ||
| name = image.split("/")[-1] | ||
| return ":" not in name or name.endswith(":latest") |
There was a problem hiding this comment.
Private-named helper imported across public modules
_is_latest_tag uses the single-underscore convention to signal a private/internal symbol, but it is now imported and used in three separate command modules (logs.py, proxy.py, and run.py). Consider renaming it to is_latest_tag (without the leading underscore) to reflect that it is intentionally part of the module's public surface.
| def _is_latest_tag(image: str) -> bool: | |
| """Return True when *image* uses the ``latest`` tag (explicitly or by omission).""" | |
| name = image.split("/")[-1] | |
| return ":" not in name or name.endswith(":latest") | |
| def is_latest_tag(image: str) -> bool: | |
| """Return True when *image* uses the ``latest`` tag (explicitly or by omission).""" | |
| name = image.split("/")[-1] | |
| return ":" not in name or name.endswith(":latest") |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/vibepod/commands/run.py (2)
26-26: Consider making_is_latest_taga public helper.The function
_is_latest_tagis imported across multiple modules (run.py,proxy.py,logs.py), but its leading underscore conventionally marks it as private. Consider renaming it tois_latest_tagto reflect its cross-module usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/vibepod/commands/run.py` at line 26, The helper function _is_latest_tag is used across modules but named as private; rename the function to is_latest_tag in its defining module and update all imports/usages (e.g., in run.py, proxy.py, logs.py) to import is_latest_tag instead of _is_latest_tag; ensure the function signature and behavior remain unchanged and run tests/linters to catch any remaining references to _is_latest_tag.
326-327: Consider adding an info message for consistency.In
proxy.py, the update check logs"Checking for proxy image updates…"before callingpull_if_newer. Here it's silent. For consistency and user visibility, consider adding a similar info message.♻️ Proposed fix
if _is_latest_tag(proxy_image): + info("Checking for proxy image updates…") manager.pull_if_newer(proxy_image)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/vibepod/commands/run.py` around lines 326 - 327, Add a user-visible info log before attempting to pull the proxy image for consistency: inside the if block that checks _is_latest_tag(proxy_image) in run.py, call the same logger/info used in proxy.py (e.g., processLogger.info or the module's logger) with "Checking for proxy image updates…" prior to invoking manager.pull_if_newer(proxy_image) so the update check is logged the same way as in proxy.py.tests/test_run.py (1)
174-178: Consider adding unit tests for_is_latest_tag.The new
_is_latest_taghelper indocker.pyis used across multiple commands but lacks dedicated unit tests. Consider adding tests to cover edge cases like registry URLs with ports (e.g.,registry.example.com:5000/image:latest), images without tags, and explicit non-latest tags.Do you want me to generate unit tests for
_is_latest_tagor open an issue to track this?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_run.py` around lines 174 - 178, Add unit tests for the helper function _is_latest_tag to cover missing and tricky tag parsing cases: create tests that call docker._is_latest_tag for (1) an image with explicit :latest (e.g., "repo/image:latest"), (2) an image with a registry including a port and :latest (e.g., "registry.example.com:5000/image:latest"), (3) an image with no tag (e.g., "repo/image") and assert the expected behavior, (4) an image with an explicit non-latest tag (e.g., "repo/image:1.2.3") and (optionally) (5) an image referenced by digest (e.g., "repo/image@sha256:..."); assert True for latest cases and False for non-latest cases, using the _is_latest_tag function name to locate the code under test. Ensure tests are added to the test suite and use clear descriptive test names for each edge case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/vibepod/commands/run.py`:
- Line 26: The helper function _is_latest_tag is used across modules but named
as private; rename the function to is_latest_tag in its defining module and
update all imports/usages (e.g., in run.py, proxy.py, logs.py) to import
is_latest_tag instead of _is_latest_tag; ensure the function signature and
behavior remain unchanged and run tests/linters to catch any remaining
references to _is_latest_tag.
- Around line 326-327: Add a user-visible info log before attempting to pull the
proxy image for consistency: inside the if block that checks
_is_latest_tag(proxy_image) in run.py, call the same logger/info used in
proxy.py (e.g., processLogger.info or the module's logger) with "Checking for
proxy image updates…" prior to invoking manager.pull_if_newer(proxy_image) so
the update check is logged the same way as in proxy.py.
In `@tests/test_run.py`:
- Around line 174-178: Add unit tests for the helper function _is_latest_tag to
cover missing and tricky tag parsing cases: create tests that call
docker._is_latest_tag for (1) an image with explicit :latest (e.g.,
"repo/image:latest"), (2) an image with a registry including a port and :latest
(e.g., "registry.example.com:5000/image:latest"), (3) an image with no tag
(e.g., "repo/image") and assert the expected behavior, (4) an image with an
explicit non-latest tag (e.g., "repo/image:1.2.3") and (optionally) (5) an image
referenced by digest (e.g., "repo/image@sha256:..."); assert True for latest
cases and False for non-latest cases, using the _is_latest_tag function name to
locate the code under test. Ensure tests are added to the test suite and use
clear descriptive test names for each edge case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91959aa5-196a-4cd9-aa58-3334e38e27ff
📒 Files selected for processing (5)
src/vibepod/commands/logs.pysrc/vibepod/commands/proxy.pysrc/vibepod/commands/run.pysrc/vibepod/core/docker.pytests/test_run.py
1 participant
Eensures that Docker images tagged as
latestare checked for updates before container startup for proxy and datasetteSummary by CodeRabbit
New Features
--paste-imagesflag to enable direct image pasting into applications via X11 support.Bug Fixes
Tests