Skip to content

Merge web and gateway images; split up server and celery; enable docker to be run as non-root#234

Merged
MarcelGeo merged 4 commits intoMerginMaps:masterfrom
camptocamp:GEO-7519-Merginmaps-Helm-Chart-Clean
Sep 24, 2024
Merged

Merge web and gateway images; split up server and celery; enable docker to be run as non-root#234
MarcelGeo merged 4 commits intoMerginMaps:masterfrom
camptocamp:GEO-7519-Merginmaps-Helm-Chart-Clean

Conversation

@domfre
Copy link
Contributor

@domfre domfre commented Jun 4, 2024

Merge web and gateway images; split up server and celery; enable docker to be run as non-root

@domfre domfre changed the title Merge web and gateway images; split up server and celery; enable dock… Merge web and gateway images; split up server and celery; enable docker to be run as non-root Jun 4, 2024
@MarcelGeo MarcelGeo requested a review from varmar05 June 11, 2024 07:51
Copy link
Collaborator

@MarcelGeo MarcelGeo left a comment

Choose a reason for hiding this comment

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

Hi. Thanks for PR :) 🥇

  • Celery split is ok in docker-compose
  • We can move other changes to development docker-compose instead of official docker compose for deployment.
  • we want to stay in gateway instead of have one nginx proxy merged with frontend image.

- db
networks:
- merginmaps
proxy:
Copy link
Collaborator

Choose a reason for hiding this comment

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

for production is more reasonable to have gateway. Frontend is just one container in stack ...

Copy link

Choose a reason for hiding this comment

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

@MarcelGeo I actually disagree on this point, could we discuss it a bit (here or in a meet)? Having 2 nginx servers, one for routing and one for statically serving the webapp actually uses more ram (you need 2 containers), degrades (minimally) the services as an additional request is sent and processed by the second nginx, and makes the deployment on k8s more cumbersome.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it depends on deployment type, I know there are some users already using host nginx as both, proxy and for TLS termination. Also there are users using dockerized proxy as stated in current docker-compose example, so I am bit worried about such changes for inexperienced users.

I agree it could be merged and you guys can certainly do that for your k8s deployments but I would prefer here in public repo to keep is as is even if not optimal. We can instead maybe provide some optimization hints instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with @varmar05 ... It should be useful to have volume for web-app (a.k.a dashboard) nginx config and upgrade It in custom deployment.

@domfre domfre force-pushed the GEO-7519-Merginmaps-Helm-Chart-Clean branch from 3656289 to 8f05b18 Compare July 11, 2024 07:37
@domfre domfre force-pushed the GEO-7519-Merginmaps-Helm-Chart-Clean branch from 9801306 to 50a6045 Compare July 11, 2024 14:10
Copy link

@danduk82 danduk82 left a comment

Choose a reason for hiding this comment

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

@domfre this looks fine from my side. @MarcelGeo this should implement your requested changes, could you review it again please?

@MarcelGeo
Copy link
Collaborator

@domfre this looks fine from my side. @MarcelGeo this should implement your requested changes, could you review it again please?

Thanks @danduk82 and @domfre ... Let me wait for @varmar05 to review nginx web and gateway ... rest of your changes looks brilliant.

@MarcelGeo MarcelGeo requested a review from danduk82 September 2, 2024 14:16
Copy link

@danduk82 danduk82 left a comment

Choose a reason for hiding this comment

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

LGTM

@MarcelGeo
Copy link
Collaborator

LGTM

There's one unresolved discussion about nginx gateway... could we improve It?

@MarcelGeo MarcelGeo merged commit 4afd22a into MerginMaps:master Sep 24, 2024
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.

5 participants