Configurable Geocoder (fix #210) #315

borrob · 2020-01-31T16:17:49Z

An implementation for issue #210: Configurable Geocoder

The plugin system is used to create a configurable geocoder. The geocoder plugin class and specific parameters are specified in the config_main.py or config_site.py. The base setup refers to the ip-api.com that is already used at the moment.

This PR provides three plugins: using a geocode service on the web via a GET request and via a POST request. The third geocoder returns a (configurable) fixed location which might be useful for intranet/on prem installations of GHC.

I did not check compatibility with the other two open PRs, but conflicts (if any) should be minor.

Add a base class for geocode plugins and an implementation for web-based geocoders (not yet finished).

This is usefull for running GHC in a private environment where geocoding the server does not make any sense.

justb4 · 2020-01-31T21:03:02Z

Need some time to dive in. At least the build failure is not from this PR, but a test that Probes the Resource https://demo.pygeoapi.io/master. That Resource has been failing I now see since jan 28, 2020 10:55 GMT. See https://demo.geohealthcheck.org/resource/165
Latest commit of GHC master and thus demo.geohealthcheck.org instance is from dec 2019. Need to investigate. The error signaled is Validate OpenAPI Compliance:HTTP Error 404: Not Found.

Dived into this, was quite a route among various products/components. All described here: geopython/pygeoapi#356 ...also to note @tomkralidis

justb4 · 2020-02-03T09:11:30Z

opengeospatial/ogcapi-processes#63 was cause and fixed so build shoild pass now.

borrob · 2020-02-04T19:50:57Z

Great find and I am happy it all works now. All tests pass.

justb4 · 2020-10-26T10:20:37Z

docs/install.rst

@@ -81,6 +81,7 @@ Install
  # - GHC_REQUIRE_WEBAPP_AUTH  # optional: to require authentication to access webapp
  # - GHC_SMTP  # if GHC_NOTIFICATIONS is enabled
  # - GHC_MAP  # or use default settings
+  # - GEOIP  # or use the feault settings


typo: "default"

Fixed in 9699493

justb4 · 2020-10-26T10:21:10Z

docs/config.rst

@@ -42,6 +42,7 @@ The configuration options are:
  - **centre_long**: Centre longitude for homepage map
  - **maxzoom**: maximum zoom level
  - **subdomains**: available subdomains to help with parallel requests
+- **GEOIP**: configuration for the geolocater service plugin. Default is the ip-api.com api.


geolocater should be geolocator, multiple files.

I feel stupid.

Fixed with: e8868a1

justb4 · 2020-10-26T10:21:44Z

docs/plugins.rst

@@ -10,7 +10,8 @@ one may want to perform. Hence developers can extend or even replace
 the GHC standard Plugins with custom implementations.

 Two Plugin types exist that can be extended: the `Probe` and `Check` class.
-In v0.7.0 also plugins for Resource Authentication, `ResourceAuth`, were added.
+In v0.7.0 also plugins for Resource Authentication, `ResourceAuth`, were added
+and in v0.8.0 the geocoder plugin was introduced.


will hopefully be 0.8.1

Made a reference to 0.9 with commit 4fa7da7

justb4

Good to see the Plugin system used!
Main comment/changes needed: any config change also requires changes to the config for Docker deployment. Not handy but Docker uses env-vars a lot. See the files:
config_site.py and default env vars in Dockerfile. Changes will be bit similar to SMTP config convention.

borrob · 2020-10-27T21:07:41Z

Thanks for the review work! I'll try to fix things up, but I'm quite busy lately, so it might take some time.

justb4 · 2020-10-27T22:22:35Z

@borrob no sweat! just keep your PR current with master, more changes upcoming in other PRs, and take your time!

Fixed Conflicts: GeoHealthCheck/util.py

borrob · 2021-04-09T19:36:10Z

Good to see the Plugin system used!
Main comment/changes needed: any config change also requires changes to the config for Docker deployment. Not handy but Docker uses env-vars a lot. See the files:
config_site.py and default env vars in Dockerfile. Changes will be bit similar to SMTP config convention.

I fixed the easy ones. I'll start to check up on the config changes.

borrob · 2021-04-09T20:23:23Z

Good to see the Plugin system used!
Main comment/changes needed: any config change also requires changes to the config for Docker deployment. Not handy but Docker uses env-vars a lot. See the files:
config_site.py and default env vars in Dockerfile. Changes will be bit similar to SMTP config convention.

I fixed the easy ones. I'll start to check up on the config changes.

@justb4 Can you be a bit more specific? I changed the config_main.py here, the config for the docker file config_site.py here, and it is described in the documentation. Or would you rather have it configured in the Docker env-var (Dockerfile and overruled in ghc.env for example)?

justb4 · 2021-04-10T09:59:22Z

@borrob the reason for the specific Docker var setup is to enable overruling the default GHC configuration in config_site.py via env vars. For example when deploying in Kubernetes or other environments where there is no docker-compose nor ghc.env local Docker volume-mount possible.

The convention would be the same as for SMTP. Given the current GEOIP config this would require 3 env vars, e.g.
GHC_GEOIP_URL, GHC_GEOIP_LATFIELD, GHC_GEOIP_LONFIELD.

Set defaults in the Dockerfile:

ENV LC_ALL="en_US.UTF-8" \
.
.
	GHC_GEOIP_URL='http://ip-api.com/json/{hostname}' \
	GHC_GEOIP_LATFIELD='lat' \
	GHC_GEOIP_LONFIELD='lon' \
.
.

And then in the config_site.py use values from environment (either the default from the Dockerfile or an overruled value):

GEOIP = {
    'plugin': 'GeoHealthCheck.plugins.geocode.webgeocoder.HttpGetGeocoder',
    'parameters': {
        'geocoder_url':  os.environ['GHC_GEOIP_URL'],
        'lat_field':  os.environ['GHC_GEOIP_LATFIELD'],
        'lon_field':  os.environ['GHC_GEOIP_LONFIELD']
}

Feels like duplicate work, but this has evolved historically and came from Kubernetes deployment. This convention is very common for many tools. Hence also the convention to start env varnames with GHC_

justb4 · 2021-04-12T08:59:22Z

Looks good now! I propose to first finalize #349 and I will merge #347 and then this one.

tomkralidis · 2021-04-12T10:34:27Z

Dockerfile

@@ -44,6 +44,9 @@ ENV LC_ALL="en_US.UTF-8" \
 	GHC_SMTP_SSL=False \
 	GHC_SMTP_USERNAME=None \
 	GHC_SMTP_PASSWORD=None \
+    GHC_GEOIP_URL='http://ip-api.com/json/{hostname}' \


Can we align/indent accordingly?

Aaarrgh. tab vs space discussion.
Fixed with 39183ac.

tomkralidis · 2021-04-12T10:35:27Z

GeoHealthCheck/geocoder.py

@@ -0,0 +1,31 @@
+from plugin import Plugin


Can we add a license header?

Fixed with 7bdf022.

tomkralidis · 2021-04-12T10:35:48Z

GeoHealthCheck/plugins/geocode/fixedlocation.py

@@ -0,0 +1,49 @@
+from GeoHealthCheck.geocoder import Geocoder


License header here as well

Fixed with 7bdf022.

tomkralidis · 2021-04-12T10:36:03Z

GeoHealthCheck/plugins/geocode/webgeocoder.py

@@ -0,0 +1,200 @@
+import requests


License header

Fixed with 7bdf022.

borrob · 2021-04-12T17:40:48Z

Thanks for all the review work and the explanation on the docker variables.

justb4 · 2021-04-15T10:17:31Z

GeoHealthCheck/geocoder.py

+        :param hostname string: the hostname of the server for which we want
+                                the coords.
+
+        TOOD: return result as tuple with locatin in lat-lon like: (52.4, 21.0)


typos:
TOOD --> TODO
locatin --> location

I should pay more attention to my typing.
Fixed with 0c377b0

justb4 · 2021-04-15T10:43:24Z

Ok, PRs #349 #347 are now in. Maybe you want to merge those in to get the green light on tests first.
So AFAICT a default config is in place, so upgrades should be smooth. You have also tested the Docker image?

BTW Whenever we merge in master branch the Docker image latest is automatically built and deployed as https://demo.geohealthcheck.org .

Need some time for full review...

borrob · 2021-04-15T19:25:39Z

Ok, PRs #349 #347 are now in. Maybe you want to merge those in to get the green light on tests first.
So AFAICT a default config is in place, so upgrades should be smooth. You have also tested the Docker image?

BTW Whenever we merge in master branch the Docker image latest is automatically built and deployed as https://demo.geohealthcheck.org .

Need some time for full review...

Great!
I merged to new master to this PR (e200a8a) and I should get flake8 into my workflow before I commit.
I ran all the tests and I did make a (local) docker build. The app runs and all seems fine...

justb4 · 2021-04-26T13:07:09Z

Thanks @borrob !

borrob added 12 commits January 26, 2020 11:49

Setting up plugin system for geocoder

237d591

Add a base class for geocode plugins and an implementation for web-based geocoders (not yet finished).

Add a fixed location geocoder

4a67072

This is usefull for running GHC in a private environment where geocoding the server does not make any sense.

Update init call

b99733e

Get request string with template

f9b8911

Use new geocoder structure

4b6f4ad

Fix passing params

de552cd

Fix online geocoder

7206974

Update config to user geocoder plugin

731084c

Update config files

b9c9475

Update docs with geocoder plugin

2058556

Update docs and lifecycle

285e83c

Fix flake 8 errors

51605f1

justb4 mentioned this pull request Jan 31, 2020

404 error in crawling wps-rest OpenAPI YAML schemas opengeospatial/ogcapi-processes#63

Closed

Empty commit to force travis

80ce73c

borrob self-assigned this Feb 4, 2020

borrob added the enhancement label Feb 4, 2020

borrob added this to the Version 0.8.0 milestone Feb 4, 2020

justb4 modified the milestones: Version 0.8.0, Version 0.8.1 Jul 7, 2020

justb4 self-requested a review October 26, 2020 10:12

justb4 reviewed Oct 26, 2020

View reviewed changes

justb4 requested changes Oct 26, 2020

View reviewed changes

justb4 removed this from the Version 0.8.1 milestone Nov 2, 2020

justb4 added this to the Version 0.9.0 milestone Nov 2, 2020

borrob added 4 commits April 9, 2021 20:58

Merge branch 'master' into geocoder

4bb0119

Fixed Conflicts: GeoHealthCheck/util.py

Fix typo

9699493

Fix typo geolocater -> geolocator

e8868a1

Update reference to version number

4fa7da7

Fix flake8 issue

0e80a09

Setup environment variables for docker setup.

193d281

tomkralidis self-requested a review April 12, 2021 10:33

tomkralidis requested changes Apr 12, 2021

View reviewed changes

borrob added 2 commits April 12, 2021 19:02

Fix indentation (tab vs space)

39183ac

Add license header

7bdf022

justb4 reviewed Apr 15, 2021

View reviewed changes

borrob added 3 commits April 15, 2021 20:56

Merge branch 'master' into geocoder

e200a8a

Fix typos

0c377b0

Fix flake8

a2a86eb

Merge branch 'master' into geocoder

6944d70

justb4 merged commit 56b7a5e into geopython:master Apr 26, 2021

borrob deleted the geocoder branch April 26, 2021 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable Geocoder (fix #210) #315

Configurable Geocoder (fix #210) #315

borrob commented Jan 31, 2020

justb4 commented Jan 31, 2020 •

edited

justb4 commented Feb 3, 2020

borrob commented Feb 4, 2020

justb4 Oct 26, 2020

borrob Apr 9, 2021

justb4 Oct 26, 2020

borrob Oct 27, 2020

borrob Apr 9, 2021

justb4 Oct 26, 2020

borrob Apr 9, 2021

justb4 left a comment

borrob commented Oct 27, 2020

justb4 commented Oct 27, 2020 •

edited

borrob commented Apr 9, 2021

borrob commented Apr 9, 2021

justb4 commented Apr 10, 2021

justb4 commented Apr 12, 2021

tomkralidis Apr 12, 2021

borrob Apr 12, 2021 •

edited

tomkralidis Apr 12, 2021

borrob Apr 12, 2021 •

edited

tomkralidis Apr 12, 2021

borrob Apr 12, 2021 •

edited

tomkralidis Apr 12, 2021

borrob Apr 12, 2021 •

edited

borrob commented Apr 12, 2021

justb4 Apr 15, 2021

borrob Apr 15, 2021

justb4 commented Apr 15, 2021

borrob commented Apr 15, 2021

justb4 commented Apr 26, 2021

		@@ -0,0 +1,49 @@
		from GeoHealthCheck.geocoder import Geocoder

Configurable Geocoder (fix #210) #315

Configurable Geocoder (fix #210) #315

Conversation

borrob commented Jan 31, 2020

justb4 commented Jan 31, 2020 • edited

justb4 commented Feb 3, 2020

borrob commented Feb 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justb4 left a comment

Choose a reason for hiding this comment

borrob commented Oct 27, 2020

justb4 commented Oct 27, 2020 • edited

borrob commented Apr 9, 2021

borrob commented Apr 9, 2021

justb4 commented Apr 10, 2021

justb4 commented Apr 12, 2021

Choose a reason for hiding this comment

borrob Apr 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borrob Apr 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borrob Apr 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borrob Apr 12, 2021 • edited

Choose a reason for hiding this comment

borrob commented Apr 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justb4 commented Apr 15, 2021

borrob commented Apr 15, 2021

justb4 commented Apr 26, 2021

justb4 commented Jan 31, 2020 •

edited

justb4 commented Oct 27, 2020 •

edited

borrob Apr 12, 2021 •

edited

borrob Apr 12, 2021 •

edited

borrob Apr 12, 2021 •

edited

borrob Apr 12, 2021 •

edited