fix: Update Java ADK docs to reflect updated agent dir #830
Conversation
There was a problem hiding this comment.
Thanks for the PR / contribution and for linking to the original issue in the adk-java repo! ❤️
I tested the full getting started flow with your commands, and everything is working as expected.
I left one comment for discussion just to understand why target and . are both working in my dev environment. But using . as you suggested here is a better pattern to use as the base location for the search.
cc @joefernandez for additional review.
docs/get-started/java.md
Outdated
| mvn compile exec:java \ | ||
| -Dexec.mainClass="com.google.adk.web.AdkWebServer" \ | ||
| -Dexec.args="--adk.agents.source-dir=target --server.port=8000" | ||
| -Dexec.args="--adk.agents.source-dir=. --server.port=8000" |
There was a problem hiding this comment.
I wonder why --adk.agents.source-dir=target is working in my environment (where the compiled classes end up in target/classes/com/example/agent) but was not working for you and others in google/adk-java#456? Do your compiled classes end up in a different location? Asking since we want to make sure we're matching common dev environments as accurately as possible.
There was a problem hiding this comment.
DO NOT MERGE. The existing command is tested and verified working. An update like this requires that we fully retest quickstart with the update before updating. DO NOT MERGE.
There was a problem hiding this comment.
Ah, I had update this for consistency with the other change, but I am ok with keeping this one as is since it is WAI. Reverting this change.
| mvn exec:java \ | ||
| -Dexec.mainClass="com.google.adk.web.AdkWebServer" \ | ||
| -Dexec.args="--adk.agents.source-dir=src/main/java" \ | ||
| -Dexec.args="--adk.agents.source-dir=. --server.port=8080" |
There was a problem hiding this comment.
We should be consistent across both pages and either use port 8000 (to match the Python getting started guide) or port 8080 (to match the default when using adk web with Java). I'd lean towards using 8080 in both since it's the default.
There was a problem hiding this comment.
This quickstart change should be retested before merging.
There was a problem hiding this comment.
I'll revert the port specification to just focus on getting the critical change in this PR through.
| If you do not see "science-app" in the dropdown menu, make sure you | ||
| are running the `mvn` command at the location where your Java source code | ||
| is located (usually `src/main/java`). | ||
| are running the `mvn` command from the root of your maven project. |
There was a problem hiding this comment.
[No change needed]
Thanks for this more general clarification!
Resolving issues raised in google/adk-java#456 (comment)
|
@joefernandez ptal for re-review. I just updated the single line definitively raising issues for users as originally reported. |
3 participants
Resolving issues raised in google/adk-java#456 (comment)