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

Docker #3

Merged
merged 16 commits into from
Nov 24, 2023
Merged

Docker #3

merged 16 commits into from
Nov 24, 2023

Conversation

BWibo
Copy link
Member

@BWibo BWibo commented Nov 21, 2023

This PR adds a build workflow and Dockerfile for a Docker image for the citydb-tool.

Features

@BWibo BWibo marked this pull request as ready for review November 21, 2023 14:57
@BWibo
Copy link
Member Author

BWibo commented Nov 21, 2023

@clausnagel I don't have any rights on this repo jet. That's why the PR is coming from a fork.

@clausnagel @yaozhihang @thomashkolbe

The image works the same way the old impexp image did:

docker run --rm --name impexp [-i -t] \
    [-e CITYDB_HOST=the.host.de] \
    [-e CITYDB_PORT=5432] \
    [-e CITYDB_NAME=theDBName] \
    [-e CITYDB_SCHEMA=theCityDBSchemaName] \
    [-e CITYDB_USERNAME=theUsername] \
    [-e CITYDB_PASSWORD=theSecretPass] \
    [-v /my/data/:/data] \
  3dcitydb/citydb-tool[:TAG] COMMAND

You can use these images for testing already:

  • bwibo/citydb-tool:edge
  • ghcr.io/bwibo/citydb-tool:edge

There are some points to discuss:

  • Where should I document the new image? For now, I suggest just adding a section in README.md of this repo. Then everybody can test it, until the new docs are ready.
  • @clausnagel Are the ENV vars listed above still the same?
  • I switched the base image to https://hub.docker.com/_/eclipse-temurin, because OpenJDK is deprecated. There are other options too (see here for instance). Probably, this base image will be fine. It offers support for all common platforms and seems to be updated regularly.
  • We might switch to Java 17 or 21, see Java 11 EOL #2
  • Do we have use for docker images built for pushes to PRs?
  • @thomashkolbe You are using a ARM64 arch Mac, right? The image should now natively support this (see here). It would be great if you could test it and confirm that this works and ARM64 is not emulated.

@clausnagel
Copy link
Member

Hi @BWibo, thanks for this PR. I try to answer all your questions below.

  • Your access rights to the repository should be fixed. Please confirm.
  • Documentation in the README until we find a proper new place is fine with me.
  • The ENV variables are ok except for CITYDB_TYPE. We currently only support PostgreSQL, so we have skipped this ENV variable for the moment.
  • Eclipse Temurin as base image is fine with me.
  • For the Docker image, we can easily use Java 17 or 21. For the source code in general, I will answer in Java 11 EOL #2 directly.
  • I personally don't need Docker images built for pushes in a PR.

@yaozhihang
Copy link
Member

The docker image build and running the citydb-tool work very nice in my tests on a WIN10 PC.

@BWibo
Copy link
Member Author

BWibo commented Nov 24, 2023

From my side this is ready. I added a workflow to build release images too. To make it work, we need to use versioning conforming to semver. E.g. 0.5.0-beta, 0.5.0

It works like this:

  • Images are built for releases and pre-releases.
    • If an image is a release, it will be tagged as latest too.
    • If it is a pre-release (checkbox in Github UI release page), it will not be tagged as latest.

Note: The workflow does not apply to existing tags. So we need to create a new release to build a first release image.

@BWibo
Copy link
Member Author

BWibo commented Nov 24, 2023

Ah wait, there is one small thing to fix.

@BWibo BWibo merged commit d4887d2 into 3dcitydb:main Nov 24, 2023
@BWibo
Copy link
Member Author

BWibo commented Nov 24, 2023

@clausnagel @yaozhihang Oh damn, I just accidentally pushed to main. Two remotes are the core of all evil! ;-)
I force pushed main back to where I believe it was. Can you please check it's the correct commit? And sry.

@BWibo
Copy link
Member Author

BWibo commented Nov 24, 2023

I could not manage to re-open this. It is now: #4

@BWibo BWibo mentioned this pull request Nov 24, 2023
4 tasks
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