search

itzg / minecraft-server-charts

MIT License
279 stars 144 forks source link

Feature: Setting default container for minecraft chart #223

Closed drewburr closed 3 months ago

drewburr commented 3 months ago

When using the minecraft chart with .mcbackup.enabled, the default container for commands such as exec and logs becomes the backup container, rather than the Minecraft server itself. This can sometimes made administration tedious, as I rarely need to interact with the backup container.

Kubernetes provides the kubectl.kubernetes.io/default-container annotation to control this behavior, which we could choose to set to the server container instead.

I wanted to open this up to see if this was a feature we'd be interested in, or understand if this choice was deliberate. Pease see this diff for the changes I'm considering.

itzg commented 3 months ago

Good point/question. No, it wasn't intentional and I agree that most people would probably expect to exec into the Minecraft container.

I'm on the fence about whether a specific annotation should be mapped or just leave it as an option in the existing podAnnotations. There's always a balancing act of chart helpfulness and mapping all possibilities to chart values.

As a compromise, it would be very helpful to suggest that annotation as an inline comment next to the values placeholder and/or the schema.

drewburr commented 3 months ago

My only thought against doing the optional annotation would be that currently, it's being templated using the release name. If we changed the container name to be a static value, it would be a bit more friendly for the end user to manage in values.yaml. It might also be worth seeing if flipping the order of containers[] would also change this behavior, since that would be a purely organizational change

drewburr commented 3 months ago

Did a quick test to try it out, and putting the backup container beneath the server container does make log and exec point to the server container. Seems that the top-most container definition is what matters

itzg commented 3 months ago

I see what you mean now. I didn't realize the container names are fully qualified. I don't even know why that would have been done in the first place -- I had inherited these charts from the original Helm central repo.

The corrected ordering sounds like a great solution especially given the unexpected container naming.