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

Refactor for GitHub Action #116

Merged
merged 2 commits into from
Sep 7, 2021
Merged

Refactor for GitHub Action #116

merged 2 commits into from
Sep 7, 2021

Conversation

lucernae
Copy link
Collaborator

Add GitHub action to build the following images:

Only from the develop branch:

  • kartoza/docker-osm:imposm-latest
  • kartoza/docker-osm:osmupdate-latest
  • kartoza/docker-osm:osmenrich-latest

The PR also reorganized the docker-compose file and the Makefile commands. Explanation included in the README.md

@lucernae
Copy link
Collaborator Author

The changelist for the reorganization:

  • move docker-imposm3 folder to docker-imposm so that the GH Action build matrix is simpler to publish the docker tag
  • delete .env file and move it to .example.env to let user put his .env file without being committed into the repo
  • separate docker-compose.yml file so that we have ready to use recipe for production usage (docker-compose.yml), local usage (docker-compose.develop.yml), custom build usage (docker-compose.build.yml), and service helper usage (docker-compose.pgadmin.yml and docker-compose.web.yml). Instructions to use is described in the README.md
  • Refactor Makefile to use parameter from .env file.

Copy link
Collaborator

@Gustry Gustry left a comment

Choose a reason for hiding this comment

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

Side note, why not using a single docker-compose file with the use of docker service ?

@lucernae
Copy link
Collaborator Author

Some future TODO, not implemented now, depending on how we wanted to make this stack production ready:

  • Break the docker tag into separate docker repo. Currently we can't easily uses public GitHub Action recipes due to our unconventional tag: kartoza/docker-osm:imposm-latest. Either we make our own tools to generate the tag name, or we make it simpler like: kartoza/docker-osm-imposm:latest.
  • Because of the above reason, we can't build and push a PR docker image tag (image tag for Pull Request to test).
  • Make the code dependency clear. At the moment, the Dockerfile recipe doesn't explain clearly why or how the code/binaries are needed.
  • Make the stack kubernetes-ready. At the moment, the image is based on ad-hoc deployment (imposm) and long polling process (osmupdate, and osmenrich). To make it kubernetes-ready, all the images needs to have a clear process state. For example imposm should be once-off job. osmupdate and osmenrich should a kubernetes cron-job. They all need to have an exit status.

@lucernae lucernae requested a review from timlinux August 31, 2021 10:56
@lucernae
Copy link
Collaborator Author

Hi @Gustry , sorry I haven't see your comment above when I'm writing the next comment after you.

Side note, why not using a single docker-compose file with the use of docker service ?

Which one are you referring to? Is it for commenting on why I break it apart into several files?
At the moment, the reason I do that is to make it easier to convert it into production deployment, and/or other orchestrator such as podman/kubernetes. The minimum files needed for production is the default one: docker-compose.yml. The rests are easily composeable/merged together if needed (via .env file in COMPOSE_FILE variables). The ordering matters. If you don't have existing docker-compose file from previous project, then just use the current order described in .example.env. If you do have existing customized compose file, let's say docker-compose.custom.yml, then put this one first and override as needed, for example: docker-compose.custom.yml:docker-compose.develop.yml:docker-compose.build.yml

The other reason is that for cloud deployment the volume declaration needs to be abstracted away. In the original recipe, the settings folder directly mounted from local directory in the main recipe docker-compose.yml. We can't convert the recipe using kompose to Kubernetes with that, so we separate the declaration (default to use volume, but you can use local bind mount in separate recipe).

@Gustry
Copy link
Collaborator

Gustry commented Sep 1, 2021

Thanks for your explanations @lucernae
As I said, it was just a "side note". I'm also discovering docker-compose config ;-)

Copy link
Collaborator

@NyakudyaA NyakudyaA left a comment

Choose a reason for hiding this comment

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

LGTM, we still need to also add tests these should cover

  • docker-osm - check if the importer runs successfully
  • docker-osm-enrich - check if the imported table extra details are added
  • docker-osm / docker-enrich check SSL connection parameters where we connect to PostgreSQL using FORCE_SSL settings

@lucernae
Copy link
Collaborator Author

lucernae commented Sep 7, 2021

I will merge the PR first, so we can check GH action.

@lucernae lucernae merged commit 514aad2 into develop Sep 7, 2021
@lucernae lucernae deleted the orchestration branch September 7, 2021 10:02
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.

None yet

3 participants