Skip to content
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

Adapt main README.md to Quarkus #836

Merged
merged 2 commits into from
Jan 23, 2025
Merged

Adapt main README.md to Quarkus #836

merged 2 commits into from
Jan 23, 2025

Conversation

adutra
Copy link
Contributor

@adutra adutra commented Jan 21, 2025

Needs #610 because the instructions for regtests will change.

Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall 👍 not approving because the PR is in the draft state :)

README.md Outdated
Then, you can run the regression tests using the following command:

```shell
POLARIS_HOST=localhost ./regtests/run.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: env POLARIS_HOST=localhost ./regtests/run.sh is more portable, I believe

Copy link
Contributor

@flyrain flyrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall with minor comments. Feel free to merge it after resolving them.

README.md Outdated
Comment on lines 64 to 66

For local development, you can run the following commands:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary, considering the following commands and their accompanying comments are already clear and self-explanatory?

README.md Outdated
Comment on lines 85 to 100
#### Running in Docker

Please note: there are no official Docker images for Apache Polaris yet. For now, you can build the
Docker images locally.

To build the Polaris server Docker image locally:

```shell
./gradlew clean :polaris-quarkus-server:assemble -Dquarkus.container-image.build=true
```

To run the Polaris server Docker image:

```shell
docker run -p 8181:8181 -p 8182:8182 apache/polaris:latest
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we simplify it a bit like this?

Suggested change
#### Running in Docker
Please note: there are no official Docker images for Apache Polaris yet. For now, you can build the
Docker images locally.
To build the Polaris server Docker image locally:
```shell
./gradlew clean :polaris-quarkus-server:assemble -Dquarkus.container-image.build=true
```
To run the Polaris server Docker image:
```shell
docker run -p 8181:8181 -p 8182:8182 apache/polaris:latest
```
#### Running in Docker
- `./gradlew clean :polaris-quarkus-server:assemble -Dquarkus.container-image.build=true` - To build the image locally.
- `docker run -p 8181:8181 -p 8182:8182 apache/polaris:latest` - To run the image.

README.md Outdated
Comment on lines 107 to 109
connections between a local machine and a pod within the cluster for both service and metrics
endpoints.
- Currently supported metrics and health endpoints:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we simplify a bit like this?

Suggested change
connections between a local machine and a pod within the cluster for both service and metrics
endpoints.
- Currently supported metrics and health endpoints:
connections between a local machine and a pod within the cluster for both service and health/metrics
endpoints:

README.md Outdated
Comment on lines 139 to 148
The above command will by default run Polaris with the Docker image `apache/polaris:latest`; if you
want to use a different image, you can modify the `docker-compose.yaml` file prior to running it;
alternatively, you can use the following commands to override the image:

```shell
cat <<EOF > /tmp/polaris-image.yml
services: { polaris: { image: localhost:5001/apache/polaris:latest } }
EOF
docker compose -f regtests/docker-compose.yml -f /tmp/polaris-image.yml up --build --exit-code-from regtest
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to not mention it in the main readme, either leaving for users/developers to figure out by themselves or moving this to the readme file under the directory regtests.

@adutra adutra marked this pull request as ready for review January 23, 2025 09:44
@adutra adutra changed the title [WIP] Adapt main README.md to Quarkus Adapt main README.md to Quarkus Jan 23, 2025
@flyrain flyrain merged commit 3be2084 into apache:main Jan 23, 2025
5 checks passed
@flyrain
Copy link
Contributor

flyrain commented Jan 23, 2025

Thanks @adutra for the fix. Thanks @jbonofre and @dimas-b for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants