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

Update docker-compose and other docker files #4646

Merged
merged 7 commits into from
Aug 29, 2023

Conversation

tsmock
Copy link
Contributor

@tsmock tsmock commented May 13, 2021

  • Update Makefile so it works (app -> backend/frontend, depending upon context)
  • Use an official postgis image (that is actually updated, mdillon/postgis:11 -> postgis/postgis:11-3.1 (3.1 is the postgis version)
  • Use official node images (tiangolo/node-frontend:10 -> node:14 (node 10 is no longer supported, and node 12 EOL is less than a year away).

@willemarcel
Copy link
Contributor

Thanks, @tsmock!! @d-rita Could you review it when you have the possibility?

@eternaltyro eternaltyro self-assigned this Apr 18, 2023
Copy link
Collaborator

@eternaltyro eternaltyro left a comment

Choose a reason for hiding this comment

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

Now that we have tested later versions of Node and Postgres (and Postgis), we can safely upgrade version tags for both of them.

When that's done, this PR is good to go.

docker-compose.yml Outdated Show resolved Hide resolved
scripts/docker/Dockerfile.frontend Outdated Show resolved Hide resolved
scripts/docker/Dockerfile.frontend_development Outdated Show resolved Hide resolved
@tsmock tsmock force-pushed the update-docker branch 2 times, most recently from 283a1a7 to 1abe60d Compare June 23, 2023 13:41
@tsmock
Copy link
Contributor Author

tsmock commented Jun 23, 2023

I need to do some fiddling. It has been awhile since I made this PR, so I need to reverify everything.

@sonarcloud
Copy link

sonarcloud bot commented Jun 23, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 2 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.4% 0.4% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@Aadesh-Baral
Copy link
Contributor

@tsmock Is this ready for review?

cc @eternaltyro

@tsmock
Copy link
Contributor Author

tsmock commented Aug 3, 2023

I don't remember if I decided it was ready or not. I'll rebase onto current dev and see if it all works.

I got distracted by some other stuff (mostly going through dependencies on the frontend or working on JOSM).

@tsmock
Copy link
Contributor Author

tsmock commented Aug 3, 2023

OK. I've been going over some of the code:

  • I think HEALTHCHECK --interval=60s --start-period=15s CMD ["curl", "-f", "http://localhost:8000/api/v2/system/heartbeat/", "||", "exit", "1"] will work, but I haven't verified it.

podman-compose up is working properly insofar as starting everything. I probably messed up the network configuration (I wasn't able to connect).

@Aadesh-Baral
Copy link
Contributor

podman-compose up is working properly insofar as starting everything. I probably messed up the network configuration (I wasn't able to connect).

Could you please clarify where you were unable to connect?

@dakotabenjamin
Copy link
Member

@tsmock on the HEALTHCHECK, docker compose runs the frontend/backend on port 80. In addition, I needed to change the following to get it to work:

diff --git a/docker-compose.yml b/docker-compose.yml
index 31f1db1f..bfcfb9cb 100644
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -59,4 +59,4 @@ services:
 
 networks:
   tm-web:
-    external: true
+    external: false

This doesn't have to do with your PR, I'm not even really sure why it was set as external in the first place. I think the docs for running/developing with Docker sorely need updating, but I don't think we need to require that to merge this PR. With the two changes suggested above, I think this is good to go, and we can follow up with a cleanup/clarity PR.

Signed-off-by: Taylor Smock <taylor.smock@kaart.com>
Signed-off-by: Taylor Smock <taylor.smock@kaart.com>
Signed-off-by: Taylor Smock <taylor.smock@kaart.com>
Signed-off-by: Taylor Smock <tsmock@fb.com>
Signed-off-by: Taylor Smock <tsmock@fb.com>
Signed-off-by: Taylor Smock <tsmock@meta.com>
Signed-off-by: Taylor Smock <tsmock@meta.com>
@sonarcloud
Copy link

sonarcloud bot commented Aug 16, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.5% 0.5% Duplication

Copy link
Member

@dakotabenjamin dakotabenjamin left a comment

Choose a reason for hiding this comment

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

LGTM

@dakotabenjamin dakotabenjamin dismissed eternaltyro’s stale review August 21, 2023 20:18

changes have been made

@dakotabenjamin dakotabenjamin merged commit 45bf393 into hotosm:develop Aug 29, 2023
3 checks passed
@tsmock tsmock deleted the update-docker branch August 29, 2023 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants