You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "MonkeyCanCode (via GitHub)" <gi...@apache.org> on 2025/05/19 05:38:33 UTC
[PR] Fix quickstart doc with docker compose [polaris]
MonkeyCanCode opened a new pull request, #1610:
URL: https://github.com/apache/polaris/pull/1610
This PR fixes couple issues found while trying to go through quick-start guide:
1. Doc `site/content/in-dev/unreleased/getting-started/quickstart.md` has an invalid ref to `getting-started/eclipselink/docker-compose-postgres.yml`. This is updated to `getting-started/assets/postgres/docker-compose-postgres.yml` (this is correct in `getting-started/eclipselink/README.md` but not in the public page)
2. Due to now we have 3 docker compose and 1 in a diff path, running this command can cause failure as they 2 of them have binds from local to container. The bind from local to container is relative to the current directory of the user (however, the docker-compose is using relative path which is not correct and causing failures). To fix this, I introduced `ASSETS_PATH` to point to the path of assets directory which is being used for binding.
3. Doc `getting-started/eclipselink/docker-compose-bootstrap-db.yml` has a hard-coded value of default username/password which is no longer valid after recent change in https://github.com/apache/polaris/pull/1522
4. quickstart page (https://polaris.apache.org/in-dev/unreleased/getting-started/quickstart/#docker-image) ask user to build the image then run docker compose and asks user to do expose `CLIENT_ID` and `CLIENT_SECRET`. Due to change in item 3, that is no longer valid as default value are not longer available. This will cause failure as well. To fix this, I add `.env` file to contain the original default value. This will continue to allow user to set them to diff by remove env loading for docker compose.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on code in PR #1610:
URL: https://github.com/apache/polaris/pull/1610#discussion_r2099391664
##########
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:
I think I got what you mean. I had made some changes to this doc for refactor. Please review.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #1610:
URL: https://github.com/apache/polaris/pull/1610#discussion_r2096979589
##########
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:
Sorry, this comment is in conjunction with the suggestion from the overall review's comment. We should do the following:
1. Move the export of CLIENT_ID/CLIENT_SECRET to the top of the file - the Docker Compose files will be able to intake the environment variables set in bash. (i.e. keep one reference at the top of the Quickstart page and one at the top of each of the cloud deployment pages)
2. Remove all references to setting the CLIENT_ID/CLIENT_SECRET elsewhere.
3. Add the export ASSETS_PATH to these references to setting CLIENT_ID/CLIENT_SECRET
Does this make sense?
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on code in PR #1610:
URL: https://github.com/apache/polaris/pull/1610#discussion_r2101123654
##########
site/content/in-dev/unreleased/getting-started/quickstart.md:
##########
@@ -36,9 +36,38 @@ 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
+```
+- **For standalone**: Omit the `-Dquarkus.container-image.tag` and `-Dquarkus.container-image.build` options if you do not need to build a Docker image.
+
+2. **Set the Assets Path**
+```shell
+export ASSETS_PATH=$(pwd)/getting-started/assets/
```
+3. **Set Authentication Credentials**
+
+Polaris supports multiple authentication methods, including the use of `CLIENT_ID` and `CLIENT_SECRET` environment variables. If you choose to use these credentials, you can set them as follows:
+
+```shell
+export CLIENT_ID=root
+export CLIENT_SECRET=s3cr3t
+```
+- **For Docker**: These variables are configured in the `getting-started.env` file. To use custom values, export them as shown above and remove the `--env-file` option from the `docker compose` command.
Review Comment:
I think env file is helping. Currently we have auth hard-coded in couple place such as auth info for Postgres. Later on if we want to integrate with additional services (e.g. minio/keycloak), we will need to do the same with current route. Introducing a env file is pretty helpful for having a centralized location for config/auth info imo and keep the docker compose file clean. It is not a must for this PR, but I do think it will avoid many of those issues in the long run.
Then for cloud providers piece, I think those are working on https://github.com/apache/polaris/pull/1646/files?
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #1610:
URL: https://github.com/apache/polaris/pull/1610#discussion_r2103838169
##########
site/content/in-dev/unreleased/getting-started/using-polaris.md:
##########
@@ -176,7 +179,9 @@ Finally, note that we include the `iceberg-aws-bundle` package here. If your tab
Refresh the Docker container with the user's credentials:
```shell
-docker compose -f getting-started/eclipselink/docker-compose.yml up -d
+docker compose -p polaris -f getting-started/eclipselink/docker-compose.yml stop spark-sql
+docker compose -p polaris -f getting-started/eclipselink/docker-compose.yml rm -f spark-sql
+docker compose -p polaris -f getting-started/eclipselink/docker-compose.yml up -d --no-deps spark-sql
Review Comment:
Same here, prefer to have a JDBC example.
##########
site/content/in-dev/unreleased/getting-started/using-polaris.md:
##########
@@ -237,14 +242,15 @@ org.apache.iceberg.exceptions.ForbiddenException: Forbidden: Principal 'quicksta
Refresh the Docker container with the user's credentials:
```shell
-docker compose -f getting-started/eclipselink/docker-compose.yml down trino
-docker compose -f getting-started/eclipselink/docker-compose.yml up -d
+docker compose -p polaris -f getting-started/eclipselink/docker-compose.yml stop trino
+docker compose -p polaris -f getting-started/eclipselink/docker-compose.yml rm -f trino
+docker compose -p polaris -f getting-started/eclipselink/docker-compose.yml up -d --no-deps trino
Review Comment:
I'd prefer to provide jdbc examples instead of eclipseLink ones here, we could do that in a follow-up PR though.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1610:
URL: https://github.com/apache/polaris/pull/1610#discussion_r2096357756
##########
getting-started/assets/.env:
##########
Review Comment:
Would you mind making this a regular file (not having `.` in front of the name)?.. and maybe rename to `getting-started.env`?
`.env` is loaded by many tools automatically, having default ID/secret values there can be confusing and hard to debug in case they interfere with something.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on code in PR #1610:
URL: https://github.com/apache/polaris/pull/1610#discussion_r2103738051
##########
site/content/in-dev/unreleased/getting-started/quickstart.md:
##########
@@ -36,9 +36,38 @@ 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
+```
+- **For standalone**: Omit the `-Dquarkus.container-image.tag` and `-Dquarkus.container-image.build` options if you do not need to build a Docker image.
+
+2. **Set the Assets Path**
+```shell
+export ASSETS_PATH=$(pwd)/getting-started/assets/
```
+3. **Set Authentication Credentials**
+
+Polaris supports multiple authentication methods, including the use of `CLIENT_ID` and `CLIENT_SECRET` environment variables. If you choose to use these credentials, you can set them as follows:
+
+```shell
+export CLIENT_ID=root
+export CLIENT_SECRET=s3cr3t
+```
+- **For Docker**: These variables are configured in the `getting-started.env` file. To use custom values, export them as shown above and remove the `--env-file` option from the `docker compose` command.
Review Comment:
Fine with me. The only problem is I don't have a way to test the cloud deployment (so I am assuming it should work...PR is updated). There are more issues with those inconsistencies. Lets get this merge first then we can fix other issues as the existed one is not even use-able. WDYT?
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on PR #1610:
URL: https://github.com/apache/polaris/pull/1610#issuecomment-2892295392
While there are some good call-outs in this PR of things we should change, there are also some regressions. Please do not merge this PR - give me until EOD to get comments in.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on code in PR #1610:
URL: https://github.com/apache/polaris/pull/1610#discussion_r2101123654
##########
site/content/in-dev/unreleased/getting-started/quickstart.md:
##########
@@ -36,9 +36,38 @@ 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
+```
+- **For standalone**: Omit the `-Dquarkus.container-image.tag` and `-Dquarkus.container-image.build` options if you do not need to build a Docker image.
+
+2. **Set the Assets Path**
+```shell
+export ASSETS_PATH=$(pwd)/getting-started/assets/
```
+3. **Set Authentication Credentials**
+
+Polaris supports multiple authentication methods, including the use of `CLIENT_ID` and `CLIENT_SECRET` environment variables. If you choose to use these credentials, you can set them as follows:
+
+```shell
+export CLIENT_ID=root
+export CLIENT_SECRET=s3cr3t
+```
+- **For Docker**: These variables are configured in the `getting-started.env` file. To use custom values, export them as shown above and remove the `--env-file` option from the `docker compose` command.
Review Comment:
I think env file is helping. Currently we have auth hard-coded in couple place such as auth info for Postgres. Later on if we want to integrate with additional services (e.g. minio/keycloak), we will need to do the same with current route. Introducing a env file is pretty helpful for having a centralized location for config/auth info imo and keep the docker compose file clean. It is not a must for this PR, but I do think it will avoid many of those issues in the long run.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #1610:
URL: https://github.com/apache/polaris/pull/1610#issuecomment-2892068966
> I would rather have everything contained in a single docker-compose file
I support that. I even suggested that in https://github.com/apache/polaris/pull/1470#discussion_r2070927873 :)
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #1610:
URL: https://github.com/apache/polaris/pull/1610#discussion_r2103826247
##########
site/content/in-dev/unreleased/getting-started/quickstart.md:
##########
@@ -36,9 +36,38 @@ 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
+```
+- **For standalone**: Omit the `-Dquarkus.container-image.tag` and `-Dquarkus.container-image.build` options if you do not need to build a Docker image.
+
+2. **Set the Assets Path**
+```shell
+export ASSETS_PATH=$(pwd)/getting-started/assets/
```
+3. **Set Authentication Credentials**
+
+Polaris supports multiple authentication methods, including the use of `CLIENT_ID` and `CLIENT_SECRET` environment variables. If you choose to use these credentials, you can set them as follows:
+
+```shell
+export CLIENT_ID=root
+export CLIENT_SECRET=s3cr3t
+```
+- **For Docker**: These variables are configured in the `getting-started.env` file. To use custom values, export them as shown above and remove the `--env-file` option from the `docker compose` command.
Review Comment:
Oh, the variables need to be exported prior to the bash script being run in the cloud deployments. I can re-test later tomorrow! Or worst-case, I can make the change myself tomorrow as well - not a big issue.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #1610:
URL: https://github.com/apache/polaris/pull/1610#issuecomment-2903330664
Thanks @MonkeyCanCode for the fix! Thanks @pingtimeout , @dimas-b , and @adnanhemani for the review. Let's keep fixing what's left.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #1610:
URL: https://github.com/apache/polaris/pull/1610#discussion_r2103829075
##########
site/content/in-dev/unreleased/getting-started/quickstart.md:
##########
@@ -83,11 +89,6 @@ When using a Gradle-launched Polaris instance in this tutorial, we'll launch an
For more information on how to configure Polaris for production usage, see the [docs]({{% relref "../configuring-polaris-for-production" %}}).
When Polaris is run using the `./gradlew run` command, the root principal credentials are `root` and `secret` for the `CLIENT_ID` and `CLIENT_SECRET`, respectively.
Review Comment:
Not a blocker: We could actually remove this line.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on code in PR #1610:
URL: https://github.com/apache/polaris/pull/1610#discussion_r2099424673
##########
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:
But there are couple problem with both existed route as well as the above, let me share my observations:
On terminal 1, we will run the following:
```
export ASSETS_PATH=$(pwd)/getting-started/assets/
docker compose --env-file getting-started/assets/getting-started.env \
-f getting-started/assets/postgres/docker-compose-postgres.yml \
-f getting-started/eclipselink/docker-compose-bootstrap-db.yml \
-f getting-started/eclipselink/docker-compose.yml up
```
Due to `USER_CLIENT_ID` and `USER_CLIENT_SECRET` are not yet defined at this stage, the spark-sql will fail due to fail auth (as both will resolve to empty string). So the container is in stop state.
Now on terminal 2, we do the needed work to bootstrap polaris entities and have a different principal created and run the following to set values for environment variables:
```
export USER_CLIENT_ID=xxxxx
export USER_CLIENT_SECRET=xxxx
```
and try to start the failed containers here with current command in the doc:
```
docker compose -f getting-started/eclipselink/docker-compose.yml up -d
```
This will actually crashed the first setup as two docker compose command on diff terminal/session will create two diff projects and they are trying to bind to the same ports.
Now to workaround the problem, I added `-p xxx` to specify project so two commands from diff sessions will be dealing with same resources. However, this doesn't solve the problem of created container doesn't reload (which back to my original point where this is not really a regression as it never works before). To have containers reload the env, we will need to stop/remove the container then recreate it. By doing so, it will then try to load envs from the client's terminal.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on code in PR #1610:
URL: https://github.com/apache/polaris/pull/1610#discussion_r2099206067
##########
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 think the doc only mentioned to use the following:
```
docker compose -f getting-started/eclipselink/docker-compose.yml up -d
```
as oppose to stop the spark-sql container then start again such as following:
```
docker compose -f getting-started/eclipselink/docker-compose.yml down spark-sql
docker compose -f getting-started/eclipselink/docker-compose.yml up -d
```
So let me add those in then undo the change for the above
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #1610:
URL: https://github.com/apache/polaris/pull/1610#discussion_r2100957452
##########
site/content/in-dev/unreleased/getting-started/quickstart.md:
##########
@@ -36,9 +36,38 @@ 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
+```
+- **For standalone**: Omit the `-Dquarkus.container-image.tag` and `-Dquarkus.container-image.build` options if you do not need to build a Docker image.
+
+2. **Set the Assets Path**
+```shell
+export ASSETS_PATH=$(pwd)/getting-started/assets/
```
+3. **Set Authentication Credentials**
+
+Polaris supports multiple authentication methods, including the use of `CLIENT_ID` and `CLIENT_SECRET` environment variables. If you choose to use these credentials, you can set them as follows:
+
+```shell
+export CLIENT_ID=root
+export CLIENT_SECRET=s3cr3t
+```
+- **For Docker**: These variables are configured in the `getting-started.env` file. To use custom values, export them as shown above and remove the `--env-file` option from the `docker compose` command.
Review Comment:
I think this is introducing additional mental complexity - why don't we just require everyone to export these variables and then remove the `.env` file itself?
##########
site/content/in-dev/unreleased/getting-started/quickstart.md:
##########
@@ -36,9 +36,38 @@ 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
+```
+- **For standalone**: Omit the `-Dquarkus.container-image.tag` and `-Dquarkus.container-image.build` options if you do not need to build a Docker image.
+
+2. **Set the Assets Path**
+```shell
+export ASSETS_PATH=$(pwd)/getting-started/assets/
```
+3. **Set Authentication Credentials**
+
+Polaris supports multiple authentication methods, including the use of `CLIENT_ID` and `CLIENT_SECRET` environment variables. If you choose to use these credentials, you can set them as follows:
+
+```shell
+export CLIENT_ID=root
+export CLIENT_SECRET=s3cr3t
+```
+- **For Docker**: These variables are configured in the `getting-started.env` file. To use custom values, export them as shown above and remove the `--env-file` option from the `docker compose` command.
Review Comment:
Additionally, we need to do the same thing to the Cloud Providers pages - should be a simple copy-paste change to all 3 pages.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on PR #1610:
URL: https://github.com/apache/polaris/pull/1610#issuecomment-2892655882
> This PR raises a question about how we structure the docker-compose commands. I understand the desire to reduce the amount of duplicated yaml across docker-compose files. However, for the sake of being beginner-friendly, I would rather have everything contained in a single docker-compose file, even if it slightly increases the maintenance effort (think upgrading the Postgres version). Wdyt?
I disagree on this - if you read my reply to the comment that @dimas-b linked (https://github.com/apache/polaris/pull/1470#discussion_r2070927873), there's the reason why the Dockerfiles need to stay piecewise (tldr - it doesn't have to do with duplicated YAML but with the ability to reload services when required). If you have a better way to solve that issue, I'm open to it!
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #1610:
URL: https://github.com/apache/polaris/pull/1610#discussion_r2096979589
##########
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:
Sorry, this comment is in conjunction with the suggestion from the overall comment. We should do the following:
1. Move the export of CLIENT_ID/CLIENT_SECRET to the top of the file - the Docker Compose files will be able to intake the environment variables set in bash. (i.e. keep one reference at the top of the Quickstart page and one at the top of each of the cloud deployment pages)
2. Remove all references to setting the CLIENT_ID/CLIENT_SECRET elsewhere.
3. Add the export ASSETS_PATH to these references to setting CLIENT_ID/CLIENT_SECRET
Does this make sense?
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on PR #1610:
URL: https://github.com/apache/polaris/pull/1610#issuecomment-2896707127
> > I would rather have everything contained in a single docker-compose file
>
> I support that. I even suggested that in [#1470 (comment)](https://github.com/apache/polaris/pull/1470#discussion_r2070927873) :)
If we all want to do it in a single docker compose file, I can take care this in the next PR. Both route works and there are ways around to make them work. More toward to a preference things (e.g. a single giant compose file may be hard to read and maybe we will have other service such as minio which we don't want to start out of box...but an end-user can decide if minio should be spin up by adding additional compose file to the command).
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #1610:
URL: https://github.com/apache/polaris/pull/1610#discussion_r2096972264
##########
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:
Take a look at this: https://polaris.apache.org/in-dev/unreleased/getting-started/using-polaris/#connecting-with-spark
We do ask users to refresh their Docker containers once they export the required variables.
You've correctly pointed out the chicken-and-the-egg problem we have here, but unfortunately this is how we have solved it so far - asking the user for one line ran is not completely unreasonable and we have no better known solution without breaking the Docker Compose file down even further :(
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #1610:
URL: https://github.com/apache/polaris/pull/1610#discussion_r2096694571
##########
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:
This is a regression, it should remain USER_CLIENT_ID/SECRET - these are user credentials rather than the admin credentials in CLIENT_ID/SECRET
##########
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:
Can we move this line to the top of the file with the exporting of the CLIENT_ID/SECRET, so that it only needs to be run once?
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain merged PR #1610:
URL: https://github.com/apache/polaris/pull/1610
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #1610:
URL: https://github.com/apache/polaris/pull/1610#discussion_r2103400389
##########
site/content/in-dev/unreleased/getting-started/quickstart.md:
##########
@@ -36,9 +36,38 @@ 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
+```
+- **For standalone**: Omit the `-Dquarkus.container-image.tag` and `-Dquarkus.container-image.build` options if you do not need to build a Docker image.
+
+2. **Set the Assets Path**
+```shell
+export ASSETS_PATH=$(pwd)/getting-started/assets/
```
+3. **Set Authentication Credentials**
+
+Polaris supports multiple authentication methods, including the use of `CLIENT_ID` and `CLIENT_SECRET` environment variables. If you choose to use these credentials, you can set them as follows:
+
+```shell
+export CLIENT_ID=root
+export CLIENT_SECRET=s3cr3t
+```
+- **For Docker**: These variables are configured in the `getting-started.env` file. To use custom values, export them as shown above and remove the `--env-file` option from the `docker compose` command.
Review Comment:
I personally think that we shouldn't introduce these further services directly into the Quickstart flow that we publish on the website - and so the environment file wouldn't be helpful even in the future. Those integrations will need to write their own Docker Compose files and all. But I can see your point as well. I'm not going to hold a hard opinion on this as a result.
Cloud Providers weren't covered in #1646. I do think that piece is a requirement for this PR though, we should ensure changes are being made together for both local and cloud providers, in general, or we risk getting to the point where things are broken again as they are now. Thoughts?
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on code in PR #1610:
URL: https://github.com/apache/polaris/pull/1610#discussion_r2099206067
##########
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 think the doc only mentioned to use the following:
```
docker compose -f getting-started/eclipselink/docker-compose.yml up -d
```
as oppose to stop the spark-sql container then start again such as following:
```
docker compose -f getting-started/eclipselink/docker-compose.yml down spark-sql
docker compose -f getting-started/eclipselink/docker-compose.yml up -d
```
So let me add those in then undo the change for the above
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on code in PR #1610:
URL: https://github.com/apache/polaris/pull/1610#discussion_r2096800119
##########
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.
If this is causing regression, I can change it back. But I don't think we ask user to follow the restart workflow as it is really tedious imo.
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #1610:
URL: https://github.com/apache/polaris/pull/1610#discussion_r2103829247
##########
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:
Sorry, missed this comment. But I see what's the issue here - the original Docker Compose command should've been run with the "-d" detached option so that everything runs in one terminal only. Not sure how I missed that, sorry about that. We shouldn't need to force users to run multiple terminals for any of these commands.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix quickstart doc with docker compose [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #1610:
URL: https://github.com/apache/polaris/pull/1610#discussion_r2103828412
##########
site/content/in-dev/unreleased/getting-started/quickstart.md:
##########
@@ -83,11 +89,6 @@ When using a Gradle-launched Polaris instance in this tutorial, we'll launch an
For more information on how to configure Polaris for production usage, see the [docs]({{% relref "../configuring-polaris-for-production" %}}).
When Polaris is run using the `./gradlew run` command, the root principal credentials are `root` and `secret` for the `CLIENT_ID` and `CLIENT_SECRET`, respectively.
-You can also set these credentials as environment variables for use with the Polaris CLI:
-```shell
-export CLIENT_ID=root
-export CLIENT_SECRET=secret
-```
Review Comment:
I think we still need this to enable polaris cli.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org