Skip to content

Conversation

@magneticflux-
Copy link

Closes #53

I'm currently testing this on my own deployment to see how well it works.

@magneticflux- magneticflux- force-pushed the master branch 2 times, most recently from 0f3cfb5 to d65be37 Compare March 20, 2020 20:24
@anthr76
Copy link

anthr76 commented Mar 23, 2020

Looks good!

@mide
Copy link
Owner

mide commented Mar 28, 2020

Nice - thank you for the contribution!

Everyone has different workflows, so I generally support this - I really like the way this is optional (if people don't configure the RCON variables, it won't attempt to run any RCON commands).

I'm not sure how I feel about the added complexity of easy-add and some of the copied code, I wonder if there is any other way to handle RCON - It may be that is the "best" way in which case it's fine. I did this once before in mide/minecraft but that was just for stop, and no other RCON commands.

I'll continue to think about this, but I really do appreciate the contribution.

@magneticflux-
Copy link
Author

easy-add might be able to be removed, I just added it because it was how @itzg did it in https://github.com/itzg/docker-minecraft-server/blob/fb92a74084da3ebfcd41a1043566d5c0638e7365/Dockerfile#L29-L57.

You could probably just download the rcon-cli binary and manually extract it if you're concerned about saving image size. easy-add just makes it easy to support difference architectures and versions at build-time.

As far as the code itself, it's been working fine in my own distribution and I haven't had any tile corruption since starting. I'm also using an Ofelia container to restart my overviewer container every hour.

@itzg
Copy link

itzg commented Mar 29, 2020

I thought I would just mention that yes, easy-add is bit a overkill for adding a single binary. I also put this in easy-add's README, but here's one alternative I used to use:

ADD https://github.com/itzg/rcon-cli/releases/download/1.4.7/rcon-cli_1.4.7_linux_amd64.tar.gz /tmp/rcon-cli.tgz
RUN tar -xf /tmp/rcon-cli.tgz -C /usr/local/bin rcon-cli && rm /tmp/rcon-cli.tgz

@mide
Copy link
Owner

mide commented Apr 3, 2020

How can you specify the RCON password?

@magneticflux-
Copy link
Author

rcon-cli has a --password flag that I use. I don't expose the RCON port at all, so there's no real point in a secure password.

@magneticflux-
Copy link
Author

easy-add was about 15MB. I removed it and downloaded rcon-cli manually and it's only 3MB.

@itzg Maybe you could run some kind of optimization step on the Golang binaries when you build them? Remove debug info and compress maybe? 15MB seems like a pretty big static binary for a one-file Golang program...

@itzg
Copy link

itzg commented Apr 8, 2020

easy-add was about 15MB. I removed it and downloaded rcon-cli manually and it's only 3MB.

@itzg Maybe you could run some kind of optimization step on the Golang binaries when you build them? Remove debug info and compress maybe? 15MB seems like a pretty big static binary for a one-file Golang program...

@magneticflux- I had already offered my advise to remove the use of easy-add, so I found this comment was not helpful. First of all, the easy-add binary is only 7MB if you look at the release assets here: https://github.com/itzg/easy-add/releases/tag/0.7.1 . The size of Go binaries is already a source of contention way outside my control; there's even an official blog article about their efforts to reduce that https://blog.golang.org/go1.7-binary-size . Finally, you're welcome to look into the module dependency graph and build definition of easy-add and contribute specific improvements.

@magneticflux- magneticflux- force-pushed the master branch 2 times, most recently from cbb189d to ac6167d Compare April 8, 2020 22:19
@mide
Copy link
Owner

mide commented Apr 8, 2020

I appreciate the build improvements, but can we try to keep this PR for just the rcon-cli? It's starting to expand to build improvements and shellcheck fixes. I like 'one PR per thing'.

For example, most of the other suggestions are solid and minor so I'd just merge them in, this one is a bit larger for me to consider.

@magneticflux-
Copy link
Author

@magneticflux- I had already offered my advise to remove the use of easy-add, so I found this comment was not helpful.

Sorry, I didn't mean to offend!

keep this PR for just the rcon-cli

Sure, I'll rebase with just the rcon-cli specific stuff.

@magneticflux-
Copy link
Author

I did a massive rebase and moved everything to do exclusively with Shellcheck and build improvements to #65.

I think this is the optimal way to add everything now!

@mide
Copy link
Owner

mide commented Apr 25, 2020

@magneticflux- I added a commit with some suggestions, please let me know what you think. We can revert if needed.

@magneticflux-
Copy link
Author

That cleans it up nicely, thanks!

@mide
Copy link
Owner

mide commented Apr 25, 2020

Super. I'd like to test this (I haven't yet tested against a live running server). If everything looks good, I'll merge when I get a chance.

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.

Running on a live world causes tile corruption

4 participants