-
Notifications
You must be signed in to change notification settings - Fork 2.4k
feat: more generic url #2556
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?
feat: more generic url #2556
Conversation
|
Response from ADK Triaging Agent Hello @racinmat, thank you for creating this PR! To help reviewers better understand and test your changes, could you please add a Thanks! |
|
@adk-bot I modified description of the PR by adding |
|
@seanzhou1023 Hi, would you be so kind and review this PR? |
|
@seanzhou1023 could you take a look? |
Fixes #2238.
Makes the logic changed by #2520 flexible and usable by agents defined in the same application.
Supersedes #2251 because that did not get requested for review and adk-bot was not triggered there for some reason.
This is more generic and allows you to set http or https as needed, which is crucial in larger ecosystems.
And it lets you nest the FastAPI into a subapp, because you can specify the base url including the path of the sub-app.
If the
urlparameter in agent card is empty string, it will be filled from the application. This lets you either hardcode the agent url or let it be defined from the code.I updated
to_a2atoo to allow same amount of flexibility.I know there you can already specify port, but having option for suffixes allows greater flexibility.
testing plan
The tests can be seen in the PR.
I removed the mock to actually exercise the A2A-related code which was just silently crashing until now.
Now, with this with, you can run
and the a2a agent will be exposed on the localhost. And if you will run the code in different environments, you can parameterize them now, and pass the URL e.g. from environment variable, which wasn't possible after merging #2520.
I considered the #2649 and adding a new flag
--protocolinstead of the--base_url, but then I wouldn't be able to express suffix, such ashttps://127.0.0.1:8081/adk.