-
Notifications
You must be signed in to change notification settings - Fork 0
restructured to use docker compose #8
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the pr. inline some notes and ideas
Co-authored-by: Jakob Miksch <[email protected]>
Co-authored-by: Jakob Miksch <[email protected]>
Co-authored-by: Jakob Miksch <[email protected]>
@JakobMiksch I also included an update to the query function to use SQL instead of plpgsql in this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, this is much cleaner.
IMHO we can also drop the prefix OSM_PG_FEATURESERV_
or is there a specific need to have it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just namespacing. I try to do it on all my project. If you prefer, I can remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, currently I do not see an advantage of the prefix. For me it makes the config files more verbose
|
||
Then, create the necessary function for pg_featureserv to return features via API request: | ||
|
||
```sh | ||
docker compose exec db psql -f /data/query_function.sql | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried the README and apparently the function must be created after the OSM tables, because the function references the table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we either move the creation of the function after the osm2pgsql or we revert the change of the function and handle the issue #4 in a separate PR
* Specify slim mode to keep the "middle tables" so we can enable automatic updates [(relevant osm2pgsql docs)](https://osm2pgsql.org/doc/manual.html#middle) | ||
* Specify output mode as "flex" so that we can use the flex output [(relevant osm2pgsql docs)](https://osm2pgsql.org/doc/manual.html#output-options) | ||
* Use the flex style config file (`data/flex_syle.lua`) [(relevant osm2pgsql docs)](https://osm2pgsql.org/doc/manual.html#lua-library-for-flex-output) | ||
* Specify "raw" as the prefix so that the tables being with `raw` [(relevant osm2pgsql docs)](https://osm2pgsql.org/doc/manual.html#database-layout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO we do not need this explanation. People can just read the offical docs or ask a LLM.
db: | ||
condition: service_healthy | ||
db: | ||
image: postgis/postgis:16-3.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the official postgis docker image automatically installs the tiger extension (see postgis/docker-postgis#187), which we do not need at all.
that's why I originally used the kartoza postgis image.
if we want to stick to the officical postgis image, we must mount a sql file with the content create extension postgis
to the start directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a PR on this PR that fixes this: #11 just merge or approve it if you agree
@iboates thanks for the updates, I tried it and it worked mostly as expected. See my comments inline |
Co-authored-by: Jakob Miksch <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am merging. We can fix the remaining stuff in follow up MRs
Closes #1
Closes #4
Everything is shoved into a single .env file because I have had a weird amount of problems when trying to specify non-default .env file locations.
Also changed the query to use SQL instead of plpgsql
In any case, everything should work as expected, just dockerized