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

Create new minecraft-server chart #226

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

Conversation

drewburr
Copy link
Contributor

Following discussion in #225, opening this up to contain the conversation in regards to the creation of a chart to refactor and ultimately replace the existing minecraft chart. This new chart will be named minecraft-server to pair with itzg/docker-minecraft-server.

Goals of the refactor:

  • Simplify and standardize. Refer to practices defined by industry-leading charts
  • Implement base features and allow for flexibility. Don't implement every possible feature and value.
    • Refer to the docs site to provide more comprehensive examples
  • Create a "feel" comparable to itzg/docker-minecraft-server. Using the chart shouldn't feel like learning a new tool

It was suggested that merging minecraft-bedrock into this new chart could be a goal, but it was agreed that this should be more of a long term goal. This effort should be closer to a rework of the existing chart, keeping in mind that it may need to be flexible to later include the bedrock chart.

@drewburr drewburr mentioned this pull request Aug 24, 2024
@drewburr
Copy link
Contributor Author

Some questions I ran into while doing initial rework:

  1. Do we need to support a Deployment and StatefulSet type? I've always been under the impression that Minecraft can't run dual-instance in any circumstance, so using StatefulSet seems like the right option, but other charts seem to be using Deployments with strategyType: Recreate. This effectively does the same thing as a STS, so I'm interested in knowing more about the potential use-case. IMO, supporting both seems unnecessarily complicated. My sentiment matches the below references, I feel a StatefulSet is supportive of the expectations a Minecraft server has (one instance and only ever once instance)
    Should Minecraft be a StatefulSet? #84
    Option to deploy server as StatefulSet #163

Perhaps someone like @Robbilie could help here. (Ref: #98)

  1. How, if at all, should the chart maintainers list be revised? I don't feel that I'm in a good place to make this decision, and wanted to toss it out there since there may be opportunity for cleanup.

  2. Should we build in / hint at the integrations for other charts, such as mc-router and mc-monitor? This could be things such as commented service annotations and service monitors that are disabled by default. I personally like this option, because it supports other parts of the itgz ecosystem.

@itzg
Copy link
Owner

itzg commented Aug 24, 2024

  1. I'm still leaning towards StatefulSet with no setting to go beyond one replica. A StatefulSet actually extends all the features of Deployment including being a ReplicaSet...but as you mentioned there can only ever be one replica active for the server data volume at a time.

  2. Myself and @billimek are the only active maintainers. So the two of us and yourself would be appropriate as chart maintainers IMHO

  3. Loosely integrating would be nice somehow vs bringing all of those parts over.

@itzg
Copy link
Owner

itzg commented Aug 25, 2024

That latest build failure sounds similar to something that was fixed recently

#222

Perhaps double check you started from the very latest revision or just bring any deltas.

@Robbilie
Copy link
Contributor

In regards to my past PR:

I built a software in front of the Minecraft Servers to keep them in sync. Behind this proxying software normal Minecraft Servers are used, for that I was using this chart.

Without an external way of keeping sync more than one replica will not be the desired use of this chart but as stated back then I would have otherwise had to create a fork of this chart and I was really grateful that instead we went with the replicaCount value and the big warning. I would appreciate keeping this :)

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.

3 participants