-
Notifications
You must be signed in to change notification settings - Fork 8
feat: support fallback from proxy for pull #170
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: cormick <[email protected]>
WalkthroughThis update corrects a typo in a help description string for a command-line flag and introduces a fallback mechanism when fetching a manifest from a remote repository. The fallback attempts to fetch the manifest without a proxy if the initial attempt with a proxy fails, logging the process. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PullFunction
participant RemoteClient (with Proxy)
participant RemoteClient (no Proxy)
User->>PullFunction: Request manifest fetch
PullFunction->>RemoteClient (with Proxy): Fetch manifest
alt Success
RemoteClient (with Proxy)-->>PullFunction: Manifest data
else Failure and Proxy is set
PullFunction->>PullFunction: Log failure, clear proxy
PullFunction->>RemoteClient (no Proxy): Retry fetch manifest
alt Retry Success
RemoteClient (no Proxy)-->>PullFunction: Manifest data
else Retry Failure
RemoteClient (no Proxy)-->>PullFunction: Error
end
else Failure and No Proxy
RemoteClient (with Proxy)-->>PullFunction: Error
end
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 0
🧹 Nitpick comments (1)
pkg/backend/pull.go (1)
52-66
: Good addition of proxy fallback mechanism.The implementation correctly adds a fallback path when fetching a manifest fails while using a proxy. This enhances the robustness of the pull operation by automatically attempting a direct connection when proxy connections fail.
Consider making the error message more user-friendly:
- fmt.Printf("Failed to fetch the manifest with proxy, fallback to fetch without proxy, error: %v\n", err) + fmt.Printf("Proxy connection failed, attempting direct connection... (original error: %v)\n", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/modelfile/generate.go
(1 hunks)pkg/backend/pull.go
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
cmd/modelfile/generate.go (1)
64-64
: Typo correction in help text looks good.Fixed the typo in the help description string for the
--output
flag from "modelfilem" to "modelfile".
@@ -49,7 +49,21 @@ func (b *backend) Pull(ctx context.Context, target string, cfg *config.Pull) err | |||
|
|||
manifestDesc, manifestReader, err := src.Manifests().FetchReference(ctx, tag) | |||
if err != nil { | |||
return fmt.Errorf("failed to fetch the manifest: %w", err) | |||
// fallback to fetch the manifest without proxy. |
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.
You cannot get manifest via a proxy because they will be cached the content of the manifest. If the content of the manifest changed, you will get the wrong content.
Support fallback from proxy for pull operations
Description
This PR adds automatic fallback capability for proxy connections during the pull operation. When a manifest fetch fails while using a proxy, the system will automatically retry without the proxy before failing completely.
Summary by CodeRabbit