-
Notifications
You must be signed in to change notification settings - Fork 122
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
base: main
Are you sure you want to change the base?
Conversation
e85b2e1
to
3f659c3
Compare
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 Doesn't the |
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 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. |
Ok, thanks @Kritzefitz - I didn't understand that limitation / risk for Should we consider omitting the |
@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. |
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.
3f659c3
to
5f26f94
Compare
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 \ |
There was a problem hiding this comment.
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)
It's been a while since I looked at this branch, but I thought I should mention that This was done with a short-term aim of achieving network connectivity between the containers within a 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). |
I've changed my mind about this - I don't think that we should be using specific container names in our |
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:
frontend
andbackend
)grocy-pod
)app-db
)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.