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

Updated for OTP v2 #2

Closed
wants to merge 2 commits into from
Closed

Updated for OTP v2 #2

wants to merge 2 commits into from

Conversation

ikespand
Copy link

  • In Dockerfile, changed the maven link and also the base image
  • Updated ReadMe with new arguments and tutorial
  • Updated Docker-compose

Copy link
Member

@stepankuzmin stepankuzmin left a comment

Choose a reason for hiding this comment

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

Hey @ikespand

Thanks for the PR, but can you please explain your reasoning behind some changes in comments?

@@ -1,10 +1,10 @@
FROM java:8-alpine
LABEL maintainer="Stepan Kuzmin <to.stepan.kuzmin@gmail.com>"
FROM lpicanco/java11-alpine:11.0
Copy link
Member

Choose a reason for hiding this comment

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

What is your reason to change the baseimage?

-v $PWD:/graphs \
-e JAVA_OPTIONS=-Xmx4G \
urbica/otp --build /graphs
docker build -t my_docker_id/otp .
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need this step? There is ready to use urbica/otp image

Build the docker image from the Dockerfile:

```shell
docker build -t ikespand/otp .
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason changing urbica/otp to ikespand/otp?

-e JAVA_OPTIONS=-Xmx4G \
urbica/otp --basePath /data --server --analyst --autoScan --verbose
my_docker_id/otp --load /var/otp/graphs/city_name/
Copy link
Member

Choose a reason for hiding this comment

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

Same here

otp --server --autoScan --verbose
-v $PWD/graphs:/var/otp/graphs \
-e JAVA_OPTIONS=-Xmx4G \
my_docker_id/otp --build \
Copy link
Member

Choose a reason for hiding this comment

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

One can use urbica/otp image here


```shell
docker run \
-p 8080:8080 \
-v $PWD/graphs:/var/otp/graphs \
-e JAVA_OPTIONS=-Xmx4G \
urbica/otp --server --autoScan --verbose
ikespand/otp --load /var/otp/graphs/portland
Copy link
Member

Choose a reason for hiding this comment

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

Saeme question here

urbica/otp --build /var/otp/graphs/portland
-v $PWD/graphs:/var/otp/graphs \
-e JAVA_OPTIONS=-Xmx4G \
ikespand/otp --build \
Copy link
Member

Choose a reason for hiding this comment

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

Saeme question here

@tarbaig
Copy link

tarbaig commented Apr 13, 2022

If I would Adresse the raised questions could this Mr be merged? We need an top 2 image and contributing seems reasonable.

@tarbaig tarbaig mentioned this pull request Apr 13, 2022
@ikespand
Copy link
Author

@stepankuzmin : Sorry, somehow I missed all updates related to this repo. This PR can be deleted now because @tarbaig have created a new PR which answers all the quries.

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.

3 participants