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

Build spigot using buildtools #67

Merged
merged 19 commits into from
Apr 16, 2016
Merged

Build spigot using buildtools #67

merged 19 commits into from
Apr 16, 2016

Conversation

brantje
Copy link
Contributor

@brantje brantje commented Apr 8, 2016

This pull request adds the following things:

  • Spigot is now using buildtools to build the server jar, because of this, first time startup takes longer
  • Added the following ENV variables
    ENABLE_RCON
    RCON_PASSWORD
    ENABLE_QUERY
    MAX_PLAYERS
    MAX_WORLD_SIZE
    Their descriptions can be found in the readme.md which i edited.

More server settings will follow

@@ -11,5 +11,4 @@ while lsof -- /start-minecraft; do
echo -n "."
sleep 1
done
echo "...switching to user 'minecraft'"
exec sudo -E -u minecraft /start-minecraft
Copy link
Owner

Choose a reason for hiding this comment

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

The sudo is needed to avoid files in the attached volume getting created as root. I'm thinking you ran into issues with /root since $HOME might have still been referring to root's. Adding an

export HOME=/data

to the top of start-minecraft.sh might resolve that.

@itzg
Copy link
Owner

itzg commented Apr 9, 2016

I really like the new environment variable options, like RCON since that'll be beneficial for my MCCY project, especially features like this one.

Overall, I'm having second thoughts about building Spigot/Bukkit during startup. Using getspigot still seems to be the best of both worlds. They're building as recommended and they're pre-building...so the only container startup cost is downloading the appropriate version.

So...I hate to ask this, but can you revert the build related changes or if it's easier submit a new PR with just the env variable type changes?

@itzg
Copy link
Owner

itzg commented Apr 9, 2016

Oh, since the RCON protocol specifies 27015 as the default, can you add an EXPOSE to the Dockerfile for that?

@brantje
Copy link
Contributor Author

brantje commented Apr 9, 2016

Overall, I'm having second thoughts about building Spigot/Bukkit during startup. Using getspigot still seems to be the best of both worlds. They're building as recommended and they're pre-building...so the only container startup cost is downloading the appropriate version.

Currently their builds crashes when you have boots with frost walker. An you're always some versions behind, with this you can keep up with spigot
I assume it will be fixed, but i needed my server, so building from scratch seemed the best option.
Maybe add an BUILD_FROM_SOURCE flag?

And i took the default server settings from the http://minecraft.gamepedia.com/Server.properties page.

Also spigot & bukkit looked to me as one thing, all the plugins are compatible.

@itzg
Copy link
Owner

itzg commented Apr 9, 2016

Ah, providing an option to build or not is a very interesting idea. Can you proceed with that?

As for Bukkit vs Spigot, it's hard to find a definitive answer especially since I don't personally use either that much. They are definitely plugin compatible, but can't tell if they still have differences with commands and config.

@brantje
Copy link
Contributor Author

brantje commented Apr 9, 2016

Afaik there are no difference.
Also, CraftBukkit stopped the development see here.

Will add the remaining server properties, and a build from source option.

@itzg
Copy link
Owner

itzg commented Apr 14, 2016

Sorry it's taken me a while to get back to this. I'm looking at the newest round of changes now.

To use rcon use the `ENABLE_RCON` and `RCON_PASSORD` variables.
By default rcon port will be `25575` but can easily be changed with the `RCON_PORT` variable.
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if it's better to recommend they do a port mapping rather adjust the config...but then you already have the code for that so doesn't need to change.

@@ -187,6 +225,68 @@ if [ ! -e server.properties ]; then
sed -i "/motd\s*=/ c motd=$MOTD" /data/server.properties
fi

if [ -n "$ALLOW_NETHER" ]; then
Copy link
Owner

Choose a reason for hiding this comment

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

Wow, you knocked out a whole bunch of options here. A helper shell function would be good to avoid the repetition, but I can refactor that later if you don't want to mess with that.

@itzg
Copy link
Owner

itzg commented Apr 14, 2016

Hmm, GitHub thinks I commented on outdated diffs...I'll try this a different way.

cp /tmp/server.properties .

if [ -n "$WHITELIST" ]; then
echo "Creating whitelist"
sed -i "/whitelist\s*=/ c whitelist=true" /data/server.properties
sed -i "/white-list\s*=/ c white-list=true" /data/server.properties
fi

if [ -n "$MOTD" ]; then
Copy link
Owner

Choose a reason for hiding this comment

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

It looks like we need a helper function now for all the repeated sed blocks. I can refactor that later if you didn't want to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes please =), i don't have much experience with bash.

@itzg itzg merged commit a4835ec into itzg:master Apr 16, 2016
@itzg
Copy link
Owner

itzg commented Apr 16, 2016

Sorry again for the delay and thanks so much for the contribution.

I'll follow up later on and refactor that properties-set bits.

@itzg
Copy link
Owner

itzg commented Apr 16, 2016

...and pushed to hub at sha256:33b498a59d496369f2949bd843957e19f9224c5dd4178d376e9c4b46bb1582ca

@itzg itzg mentioned this pull request Apr 18, 2016
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.

None yet

3 participants