-
Notifications
You must be signed in to change notification settings - Fork 14
Choose scheme based on presence of TLS #17
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
Choose scheme based on presence of TLS #17
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR refactors how the orchestrator client determines and uses its URI scheme by detecting the presence of TLS assets, returning both the HTTP client and the appropriate scheme, and propagating that scheme through the routing and request logic instead of hard-coding “https”. Sequence diagram for scheme propagation in orchestrator requestsequenceDiagram
participant Main
participant build_orchestrator_client
participant Router
participant handle_generation
Main->>build_orchestrator_client: Call with hostname
build_orchestrator_client-->>Main: Return (client, scheme)
Main->>Router: Pass client and scheme to route handler
Router->>handle_generation: Call with scheme and client
handle_generation->>handle_generation: Build URL using scheme
handle_generation->>Orchestrator: Send request to constructed URL
Class diagram for orchestrator client and scheme propagation changesclassDiagram
class Main {
+build_orchestrator_client(hostname: &str) -> (reqwest::Client, String)
}
class build_orchestrator_client {
+returns: (reqwest::Client, scheme: String)
-detects TLS assets
-sets scheme to "https" if TLS present, else "http"
}
class handle_generation {
+scheme: String
+orchestrator_client: Arc<reqwest::Client>
+gateway_config: GatewayConfig
+route_fallback_message: Option<String>
+uses scheme to build orchestrator URL
}
Main --> build_orchestrator_client
Main --> handle_generation
build_orchestrator_client --> handle_generation : scheme
build_orchestrator_client --> handle_generation : orchestrator_client
File-Level Changes
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:
- Consider using an enum (e.g. Http vs Https) instead of a raw String for
schemeto avoid typos and improve type safety. - Centralize the orchestrator base-URL and scheme logic inside a dedicated client abstraction so you don't have to pass
schemearound in every handler. - Revisit the TLS detection logic—if you’re adding a custom CA you may still want to default to HTTPS even without a client identity, so ensure
schemematches your intended TLS scenarios.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using an enum (e.g. Http vs Https) instead of a raw String for `scheme` to avoid typos and improve type safety.
- Centralize the orchestrator base-URL and scheme logic inside a dedicated client abstraction so you don't have to pass `scheme` around in every handler.
- Revisit the TLS detection logic—if you’re adding a custom CA you may still want to default to HTTPS even without a client identity, so ensure `scheme` matches your intended TLS scenarios.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
PR image build completed successfully! 📦 PR image: |
Summary by Sourcery
Select the HTTP scheme dynamically based on the presence of TLS certificates and propagate it to orchestrator requests
Enhancements: