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

Configurable Makefile #142

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

Kritzefitz
Copy link
Contributor

@Kritzefitz Kritzefitz commented Sep 8, 2021

While working on #141 I had some ideas for further changes. But I didn't want to bloat the scope of that pull request further. So here they are (or will be, when I'm finished).

The Makefile uses various names or values that don't affect the inner workings of the grocy containers. We can make them configurable without much trouble, by using overridable variables for them. The cases I identified that should be configurable are:

  • Container names (currently frontend and backend)
  • The name of the pod (currently grocy-pod)
  • The name of the volume (currently app-db)
  • The endpoint for the published port (currently 127.0.0.1:80808)

I also noticed that some of the current names don't convey any relation to grocy, e.g. fontend could be a frontend for anything. Unfortunately podman doesn't namespace containers or volumes by the pod they belong to, so these names all end up in a namespace global to the whole podman instance. I would like to change the defaults to convey the relation to grocy:

  • frontend -> grocy-frontend
  • backend -> grocy-backend
  • app-db -> grocy-app-db

Ideally, when these changes are merged, you should be able to run multiple grocy instances on the same podman instance without collisions by using different names for all instances.

@Kritzefitz Kritzefitz marked this pull request as ready for review May 6, 2022 09:14
@Kritzefitz Kritzefitz requested a review from jayaddison May 6, 2022 09:14
@jayaddison
Copy link
Contributor

Hey @Kritzefitz, thanks! - the use case to run multiple instances of the application makes sense.

I'm a little unsure about changing the names of the containers, though. As a bit of history, what is now the frontend container was previously named nginx, and the change was made to clarify that it's not a 'plain' nginx container -- because it also hosts frontend assets (JS/CSS/HTML/etc) for the grocy application.

Doesn't the IMAGE_PREFIX (current value: docker.io/grocy) already disambiguate the fact that frontend and backend are part of grocy?

@Kritzefitz
Copy link
Contributor Author

Hi @jayaddison, my main concern is that our container names might clash with containers of completely unrelated applications. Note that container and pod names are not implicitly namespaced or prefixed by podman (note: this is different from docker-compose, which does prefix container names). The IMAGE_PREFIX is only applied to image names (which I haven't changed), not the names of the containers created by them. Since frontend and backend are rather generic names, it wouldn't be unreasonable to assume that some other project might use those names for their own containers. If they do, you would run into naming conflicts when trying to run grocy and that other application on the same podman instance.

But I'm not especially attached to the change of the default names. If you feel more comfortable leaving the names as is, just say so and I will remove the change from the PR.

@jayaddison
Copy link
Contributor

Ok, thanks @Kritzefitz - I didn't understand that limitation / risk for podman-based running containers previously.

Should we consider omitting the --name argument entirely and allowing podman to autogenerate unassociated names?

@Kritzefitz
Copy link
Contributor Author

@jayaddison, omitting the names seems reasonable, since most operations on the containers can (and in most cases should) be run on the pod, which will keep its name. I will make the necessary changes and push them when ready.

Kritzefitz and others added 4 commits May 27, 2022 17:33
Since podman doesn't have namespacing for containers or volumes, we
should prefix container and volume names, so their relation to grocy
is clear.
The way we currently setup podman containers in the Makefile, we don't
get any kind of namespacing for containers. To avoid clashes with
other names, we avoid giving the containers names at all. Most user
operations are can still refer to the stable name of the pod.
@Kritzefitz
Copy link
Contributor Author

I removed the container names.

PLATFORM ?= linux/386 linux/amd64 linux/arm/v6 linux/arm/v7 linux/arm64/v8 linux/ppc64le linux/s390x

build: manifest

create: pod
podman create \
--add-host grocy:127.0.0.1 \
--add-host frontend:127.0.0.1 \
--add-host backend:127.0.0.1 \
Copy link
Contributor

Choose a reason for hiding this comment

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

What are these hostname references used for?

(the only service-to-service connectivity I can think of at the moment is that the frontend NGINX instance is configured to pass some requests through to the backend service.. very possible I've forgotten something)

@jayaddison
Copy link
Contributor

It's been a while since I looked at this branch, but I thought I should mention that container_name entries have been re-added to the docker-compose.yml file in fde4b1a, since that is potentially relevant to some previous discussion here.

This was done with a short-term aim of achieving network connectivity between the containers within a grocy pod run using podman-compose (something I'm evaluating as a potential migration path away from docker-compose -- and that could also potentially simplify some of the Makefile).

I'm not certain yet whether it's a good idea or not to provide static container names, to be completely honest; I can see it introducing some namespace collision concerns (even within a single deployment, when scaling horizontally).

@jayaddison
Copy link
Contributor

It's been a while since I looked at this branch, but I thought I should mention that container_name entries have been re-added to the docker-compose.yml file in fde4b1a, since that is potentially relevant to some previous discussion here.

I've changed my mind about this - I don't think that we should be using specific container names in our docker-compose.yml file. Change reverted in 0da7afb.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants