-
Notifications
You must be signed in to change notification settings - Fork 114
Kraken: allow installing extension if pull fails but image is available offline #3649
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
Open
Williangalvani
wants to merge
1
commit into
bluerobotics:master
Choose a base branch
from
Williangalvani:allow_local_extension
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Kraken: allow installing extension if pull fails but image is available offline #3649
Williangalvani
wants to merge
1
commit into
bluerobotics:master
from
Williangalvani:allow_local_extension
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's guide (collapsed on small PRs)Reviewer's GuideIntroduce a local image availability check and adjust the install flow to retain and use user-committed images when a pull fails, instead of always cleaning up. Sequence diagram for extension install flow with local image fallbacksequenceDiagram
participant Kraken
participant Docker
participant Extension
Kraken->>Extension: install(clear_remaining_tags, atomic)
Extension->>Docker: pull_docker_image()
alt Pull succeeds
Extension->>Docker: Use pulled image
else Pull fails
Extension->>Docker: image_is_available_locally()
alt Image available locally
Extension->>Docker: Use local image
else Image not available
Extension->>Extension: uninstall()
Extension->>Extension: enable() (if running_ext)
Extension->>Kraken: Raise ExtensionPullFailed
end
end
Class diagram for updated Extension install logicclassDiagram
class Extension {
+install(clear_remaining_tags: bool, atomic: bool)
+_pull_docker_image(docker_auth: Optional[str])
+_image_is_available_locally() bool
}
Extension --> DockerCtx
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- After detecting the image locally, you might still need to call
running_ext.enable()so the extension actually gets started instead of silently skipping enable. - In
_image_is_available_locally, consider catching only the specific 'image not found' exception rather than swallowing all exceptions to avoid hiding other errors. - Double-check that no other cleanup path later in the code will delete the newly committed image after a failed pull, to prevent surprising user data loss.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- After detecting the image locally, you might still need to call `running_ext.enable()` so the extension actually gets started instead of silently skipping enable.
- In `_image_is_available_locally`, consider catching only the specific 'image not found' exception rather than swallowing all exceptions to avoid hiding other errors.
- Double-check that no other cleanup path later in the code will delete the newly committed image after a failed pull, to prevent surprising user data loss.
## Individual Comments
### Comment 1
<location> `core/services/kraken/extension/extension.py:180-187` </location>
<code_context>
docker_auth = f"{self.source.auth.username}:{self.source.auth.password}"
return base64.b64encode(docker_auth.encode("utf-8")).decode("utf-8")
+ async def _image_is_available_locally(self) -> bool:
+ """Check if the Docker image is available locally."""
+ try:
+ async with DockerCtx() as client:
+ await client.images.inspect(f"{self.source.docker}:{self.tag}")
+ return True
+ except Exception:
+ return False
+
async def _pull_docker_image(self, docker_auth: Optional[str]) -> AsyncGenerator[bytes, None]:
</code_context>
<issue_to_address>
**issue (bug_risk):** Catching all exceptions may mask underlying issues.
Catch only the exception(s) that indicate the image is not found, rather than all exceptions, to prevent hiding unrelated errors.
</issue_to_address>
### Comment 2
<location> `core/services/kraken/extension/extension.py:182-187` </location>
<code_context>
+ """Check if the Docker image is available locally."""
+ try:
+ async with DockerCtx() as client:
+ await client.images.inspect(f"{self.source.docker}:{self.tag}")
+ return True
+ except Exception:
</code_context>
<issue_to_address>
**suggestion:** Image reference construction may not handle digests.
This check may fail for images pulled by digest. Align the reference format with the pull logic to ensure consistent identification.
```suggestion
try:
image_ref = f"{self.source.docker}:{self.tag}" + (f"@{self.digest}" if self.digest else "")
async with DockerCtx() as client:
await client.images.inspect(image_ref)
return True
except Exception:
return False
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
79ff87e to
1543860
Compare
1543860 to
bd4233a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This allows users to commit the current container state to a new image, to save changes, and then switch the extension to this new image.
In master, the newly commited image gets deleted after the pull fails and we cleanup.
Summary by Sourcery
Enable offline installation of extensions by verifying and reusing locally available Docker images when remote pulls fail, rather than aborting and deleting the image.
New Features:
Enhancements: