Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the ADK launcher framework to improve its usability and align with standard Go practices for command-line flag handling. This change addresses critical issues such as conflicts with user-defined flags and the unsuitability of the previous "prod" configuration. By simplifying the framework and providing a dedicated utility for flag definition, it aims to make the ADK more robust and easier for developers to integrate and extend, particularly for custom deployments. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly simplifies the launcher framework by refactoring the command-line flag handling to be more idiomatic for Go applications. The removal of the universal and prod launchers, along with the consolidation of web-related sub-launchers, greatly improves the clarity and maintainability of the code. The changes are well-aligned with the stated goals in the description. I've identified one critical bug and a couple of areas for minor improvement to enhance clarity.
cmd/launcher/web/web.go
Outdated
| if err := l.SetupSubrouters(router, config); err != nil { | ||
| return fmt.Errorf("%s subrouter setup failed: %v", l.Keyword(), err) | ||
| if w.config.EnableA2A { | ||
| if err := w.apiSubLauncher.SetupSubrouters(router, config); err != nil { |
There was a problem hiding this comment.
This appears to be a copy-paste error. The A2A subrouter setup is using w.apiSubLauncher instead of w.a2aSubLauncher. This will cause the A2A functionality to be incorrectly configured or to fail entirely when enabled.
| if err := w.apiSubLauncher.SetupSubrouters(router, config); err != nil { | |
| if err := w.a2aSubLauncher.SetupSubrouters(router, config); err != nil { |
| if err := full.Run(ctx, adkFlags, config); err != nil { | ||
|
|
||
| } |
There was a problem hiding this comment.
While this is valid for an example, the empty if block can be confusing. It would be beneficial to add a comment to indicate that error handling should be present in a real application, guiding users towards best practices.
if err := full.Run(ctx, adkFlags, config); err != nil {
// In a real application, you would handle this error, e.g., by logging.
// log.Fatalf("Run failed: %v", err)
}| } | ||
|
|
||
| func DefineAPIFlags(cfg *APIConfig) { | ||
| flag.StringVar(&cfg.FrontendAddress, "adk_webui_address", "localhost:8080", "ADK WebUI address as seen from the user browser. It's used to allow CORS requests. Please specify only hostname and (optionally) port.") |
There was a problem hiding this comment.
The flag name adk_webui_address is a bit confusing here, as this file configures the API, not the WebUI. This flag is used to set the Access-Control-Allow-Origin CORS header for the API. Renaming it to something like adk_webapi_cors_origin would make its purpose clearer and avoid potential confusion with the adk_webapi_address flag defined in webui.go.
| flag.StringVar(&cfg.FrontendAddress, "adk_webui_address", "localhost:8080", "ADK WebUI address as seen from the user browser. It's used to allow CORS requests. Please specify only hostname and (optionally) port.") | |
| flag.StringVar(&cfg.FrontendAddress, "adk_webapi_cors_origin", "localhost:8080", "The origin to allow for CORS requests to the API (e.g., the address of the ADK WebUI). Please specify only hostname and (optionally) port.") |
Right now, the launcher framework has some deficiencies: * If the user defines any of their own flags (to configure APIs or tools), then the launcher immediately fails with a "flag provided but not defined" error. * It has a config named `prod` that is unsuitable for prod (no authentication). * It is written to be extremely flexible, but only provides four concrete capabilities (console, api, a2a, webui), and is unlikely to be the framework that a productionized ADK deployment uses. The first problem is the most serious; I've seen more than one engineer immediately assume that adk-go is deeply broken and switch to adk-python. The built-in Go flag package does not provide *any* capability for safely parsing partial sets of flags out of a single set of command-line arguments. This change refactors the launcher framework to be a little bit more idiomatic for the way a Go binary typically handles flags. The user will need to declare the ADK flags in their own main file, but we provide a utility function `adk.DefineFlags` to make this painless. Most of the unnecessary flexibility is removed. The prod config is removed.
ahmedtd
force-pushed
the
simplify-launcher
branch
from
555f01a to
ab5b094
Compare
1 participant
Right now, the launcher framework has some deficiencies:
prodthat is unsuitable for prod (no authentication).The first problem is the most serious; I've seen more than one engineer immediately assume that adk-go is deeply broken and switch to adk-python.
The built-in Go flag package does not provide any capability for safely parsing partial sets of flags out of a single set of command-line arguments, so what the launcher framework is currently trying to do is ill-defined.
This change refactors the launcher framework to be a little bit more idiomatic for the way a Go binary typically handles flags. The user will need to declare the ADK flags in their own main file, but we provide a utility function
adk.DefineFlagsto make this painless.Most of the unnecessary flexibility is removed.
The prod config is removed.
Closes #606