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

Dropwizardify - really now #1314

Merged
merged 51 commits into from Apr 7, 2018

Conversation

Projects
None yet
5 participants
@michaz
Member

michaz commented Mar 18, 2018

  • switch back to commit 72ef9c1
  • adapt graphhopper.sh #1319
  • additionally to -Dgraphhopper allow -Dgh (or only allow the latter?) no, we use the graphhopper.sh script for shorter and commonly used parameters
  • avoid double logging, see above
  • turn off admin service in config by default for security reasons
  • adapt docs to document new config options or provide a backward compatible way
  • add healthcheck to avoid warning at start up
  • use endpoint /maps instead of /webapp for UI demo
  • the graphhopper banner for CLI is not fully printed, one part of the second P is missing
@karussell

This comment has been minimized.

Show comment
Hide comment
@karussell

karussell Jul 7, 2017

Member

👍 Looks like a good direction :) !

Member

karussell commented on a6b4af4 Jul 7, 2017

👍 Looks like a good direction :) !

michaz added some commits Jul 24, 2017

Insert separate fields for planned/predicted departure/arrival time
Signed-off-by: Michael Zilske <michael.zilske@tu-berlin.de>
Set the expected times fields only when there is an update.
Signed-off-by: Michael Zilske <michael.zilske@tu-berlin.de>
Merge branch 'departure-arrival-times' into dropwizardify
Signed-off-by: Michael Zilske <michael.zilske@tu-berlin.de>

# Conflicts:
#	core/src/main/java/com/graphhopper/util/CmdArgs.java
#	web/src/main/java/com/graphhopper/http/SimpleRouteSerializer.java
Merge branch 'master' into dropwizardify
Signed-off-by: Michael Zilske <michael.zilske@tu-berlin.de>

# Conflicts:
#	config-example.properties
#	reader-gtfs/src/main/java/com/graphhopper/reader/gtfs/TripFromLabel.java
Bump Dropwizard version; declare J2EE dependencies (for Java 9)
Signed-off-by: Michael Zilske <michael.zilske@tu-berlin.de>
Use the same Jackson version in the client.
Signed-off-by: Michael Zilske <michael.zilske@tu-berlin.de>
@karussell

This comment has been minimized.

Show comment
Hide comment
@karussell

karussell Mar 27, 2018

Member

Fixed some more issues. The only remaining issue is "Class path contains multiple SLF4J bindings" but we can also merge first and fix later. WDYT @michaz ? Would you mind to update to master?

Member

karussell commented Mar 27, 2018

Fixed some more issues. The only remaining issue is "Class path contains multiple SLF4J bindings" but we can also merge first and fix later. WDYT @michaz ? Would you mind to update to master?

@karussell karussell self-requested a review Apr 4, 2018

karussell added some commits Apr 4, 2018

@karussell karussell merged commit d6f14bc into master Apr 7, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@karussell karussell deleted the dropwizardify2 branch Apr 7, 2018

@karussell karussell added this to the 0.11 milestone Apr 7, 2018

@karussell karussell restored the dropwizardify2 branch Apr 7, 2018

@karussell karussell deleted the dropwizardify2 branch Apr 7, 2018

@karussell karussell restored the dropwizardify2 branch Apr 7, 2018

karussell added a commit that referenced this pull request Apr 9, 2018

@karussell karussell referenced this pull request Apr 9, 2018

Closed

Review dropwizard merge more detailed #1329

3 of 3 tasks complete
@ammagamma

This comment has been minimized.

Show comment
Hide comment
@ammagamma

ammagamma Apr 12, 2018

Contributor

I used to have different run configurations in IntelliJ were I was passing graphhopper properties as program arguments, like datareader.file etc., now I need to have these in config.yml, right ? I would at least find it useful to have some kind of naming pattern or dedicated folder in .gitignore where I can put different config.yml files if it needs to be done this way, or what is your approach for this ? For example I want to run GraphHopperApplication from the IDE for debugging with certain program arguments, but to support different settings I need multiple config.yml files ?

Contributor

ammagamma commented Apr 12, 2018

I used to have different run configurations in IntelliJ were I was passing graphhopper properties as program arguments, like datareader.file etc., now I need to have these in config.yml, right ? I would at least find it useful to have some kind of naming pattern or dedicated folder in .gitignore where I can put different config.yml files if it needs to be done this way, or what is your approach for this ? For example I want to run GraphHopperApplication from the IDE for debugging with certain program arguments, but to support different settings I need multiple config.yml files ?

@michaz

This comment has been minimized.

Show comment
Hide comment
@michaz

michaz Apr 12, 2018

Member

Yes, the idea is that those are your run configurations. I'd suggest git-ignoring those files locally -- we shouldn't assume a user doesn't want to commit a configuration.

Member

michaz commented Apr 12, 2018

Yes, the idea is that those are your run configurations. I'd suggest git-ignoring those files locally -- we shouldn't assume a user doesn't want to commit a configuration.

@ammagamma

This comment has been minimized.

Show comment
Hide comment
@ammagamma

ammagamma Apr 12, 2018

Contributor

Ok, but then we should remove config.yml from .gitignore ?

config.yml

Contributor

ammagamma commented Apr 12, 2018

Ok, but then we should remove config.yml from .gitignore ?

config.yml

@ammagamma

This comment has been minimized.

Show comment
Hide comment
@ammagamma

ammagamma Apr 12, 2018

Contributor

hmm no nevermind, in this case it would be too likely that one accidentally pushes some custom config I guess

Contributor

ammagamma commented Apr 12, 2018

hmm no nevermind, in this case it would be too likely that one accidentally pushes some custom config I guess

@michaz

This comment has been minimized.

Show comment
Hide comment
@michaz

michaz Apr 12, 2018

Member

Ok, but then we should remove config.yml from .gitignore ?

I agree. Reason: Git is distributed -- what the user is commiting to might just as well be (and in my case often is) their own project repository where they commit their configuration to. .gitignore shouldn't be biased towards origin/master. :-)
But maybe in this case the benefits outweigh the philosophy, don't know.

Member

michaz commented Apr 12, 2018

Ok, but then we should remove config.yml from .gitignore ?

I agree. Reason: Git is distributed -- what the user is commiting to might just as well be (and in my case often is) their own project repository where they commit their configuration to. .gitignore shouldn't be biased towards origin/master. :-)
But maybe in this case the benefits outweigh the philosophy, don't know.

@michaz

This comment has been minimized.

Show comment
Hide comment
@michaz

michaz Apr 12, 2018

Member

Actually, I would go one step further, rename config-example.yml to config.yml, remove it from .gitignore and change it so that it actually runs when run from the project root, i.e. without the commented-out OSM filename, but with the Monaco test data.

Member

michaz commented Apr 12, 2018

Actually, I would go one step further, rename config-example.yml to config.yml, remove it from .gitignore and change it so that it actually runs when run from the project root, i.e. without the commented-out OSM filename, but with the Monaco test data.

@ammagamma

This comment has been minimized.

Show comment
Hide comment
@ammagamma

ammagamma Apr 12, 2018

Contributor

I don't think I have a strong opinion here, but why not. Only thing is that it should be documented well
, maybe even a little 'migration guide' for dropwizard to make it easier to switch. I would still find it useful to have something like local/ in .gitignore. I am using .git/info/exclude where I added local/ with some folders for maps, config files etc. now

Contributor

ammagamma commented Apr 12, 2018

I don't think I have a strong opinion here, but why not. Only thing is that it should be documented well
, maybe even a little 'migration guide' for dropwizard to make it easier to switch. I would still find it useful to have something like local/ in .gitignore. I am using .git/info/exclude where I added local/ with some folders for maps, config files etc. now

@karussell

This comment has been minimized.

Show comment
Hide comment
@karussell

karussell Apr 12, 2018

Member

I have it working with just one config.yml like before and e.g. the following VM options:

-Xmx4g -Xms4g -Dgraphhopper.graph.flag_encoders=car,foot -Dgraphhopper.datareader.file=berlin-latest.osm.pbf -Dgraphhopper.graph.location=graph-cache
Member

karussell commented Apr 12, 2018

I have it working with just one config.yml like before and e.g. the following VM options:

-Xmx4g -Xms4g -Dgraphhopper.graph.flag_encoders=car,foot -Dgraphhopper.datareader.file=berlin-latest.osm.pbf -Dgraphhopper.graph.location=graph-cache
@karussell

This comment has been minimized.

Show comment
Hide comment
@karussell

karussell Apr 12, 2018

Member

Regarding the .gitignore there is also this possibility: https://stackoverflow.com/q/7335420/194609

Not sure if this applies in your case. If not, we can surely find another solution like the local/

And yes, a migration guide should be written.

Member

karussell commented Apr 12, 2018

Regarding the .gitignore there is also this possibility: https://stackoverflow.com/q/7335420/194609

Not sure if this applies in your case. If not, we can surely find another solution like the local/

And yes, a migration guide should be written.

@ammagamma

This comment has been minimized.

Show comment
Hide comment
@ammagamma

ammagamma Apr 12, 2018

Contributor

Ok using the VM options works (and overwrites the settings from config.yml). The link you posted is the global version of using .git/info/exclude, so for me these are enough options to choose from :)

Contributor

ammagamma commented Apr 12, 2018

Ok using the VM options works (and overwrites the settings from config.yml). The link you posted is the global version of using .git/info/exclude, so for me these are enough options to choose from :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment