MonkeyCanCode commented on code in PR #1610:
URL: https://github.com/apache/polaris/pull/1610#discussion_r2096804622


##########
getting-started/assets/.env:
##########


Review Comment:
   Good suggestion. This is fixed. 



##########
getting-started/eclipselink/docker-compose.yml:
##########
@@ -82,7 +82,7 @@ services:
       --conf, "spark.sql.catalog.quickstart_catalog.type=rest",
       --conf, 
"spark.sql.catalog.quickstart_catalog.warehouse=quickstart_catalog",
       --conf, 
"spark.sql.catalog.quickstart_catalog.uri=http://polaris:8181/api/catalog";,
-      --conf, 
"spark.sql.catalog.quickstart_catalog.credential=${USER_CLIENT_ID}:${USER_CLIENT_SECRET}",
+      --conf, 
"spark.sql.catalog.quickstart_catalog.credential=${CLIENT_ID}:${CLIENT_SECRET}",

Review Comment:
   I did noticed this one last night and assuming there is intensional. 
However, going through the quickstart guide, this is supposed to be the 
credential created at the end of the polaris CLI. But `USER_CLIENT_ID` and 
`USER_CLIENT_SECRET` is never defined in this context I believed.  As an 
end-user tries to bring up setup via docker compose, there is no relation to 
the polaris CLI. 
   
   Now assuming we want to keep it this way, an end-user can't really define 
this one until polaris CLI returns the value back (which needed to bring up 
docker compose first, run polaris CLI for entities creation, then restart 
spark-sql to have this be effective). This restart workflow is really odd imo 
and we should replace it with other more automated workflow.
   
   Let me know what you think.



##########
site/content/in-dev/unreleased/getting-started/quickstart.md:
##########
@@ -36,7 +36,8 @@ cd ~/polaris
   :polaris-quarkus-admin:assemble --rerun \
   -Dquarkus.container-image.tag=postgres-latest \
   -Dquarkus.container-image.build=true
-docker compose -f getting-started/eclipselink/docker-compose-postgres.yml -f 
getting-started/eclipselink/docker-compose-bootstrap-db.yml -f 
getting-started/eclipselink/docker-compose.yml up
+export ASSETS_PATH=$(pwd)/getting-started/assets/

Review Comment:
   Not sure if I followed, this is the beginning of the quickstart page 
(https://polaris.apache.org/in-dev/unreleased/getting-started/quickstart/#docker-image).
 This is only needed for the context of docker compose. 
   
   The export of CLIENT_ID/CLIENT_SECRET is invalid I think as the current 
state (without the env) file, this won't even be able to start. 
   
   If I understand correctly, we should consider move export of 
CLIENT_ID/CLIENT_SECRET to this section (as the current docker compose file has 
no credential, so it will try to set empty string for root credential (as well 
as username, which is in-valid).
   
   The export is only needed if user doesn't want to use env file (as env file 
will load the credential in the updated command). Let me know what you think.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to