-
Notifications
You must be signed in to change notification settings - Fork 7
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 and publish Docker images #257
base: master
Are you sure you want to change the base?
Conversation
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.
Just a few nits (and questions), otherwise looks good.
The final image is around 150M in size, which is totally reasonable. Most of the size is added by the maps.
I managed to remove the patch by introducing a new server variable called |
Tested this new variable in production now, works just the way it's meant to work. |
|
||
if (servertype <= 0) { | ||
return; | ||
}; |
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.
Useless semicolon here.
conoutf("public server IP is %s", serverpublicip); | ||
} | ||
|
||
if(setupserversockets() && verbose) { |
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.
Style:
if(setupserversockets() && verbose) { | |
if (setupserversockets() && verbose) { |
@@ -438,7 +438,16 @@ namespace auth | |||
conoutf("Updating master server"); | |||
// the position of the 0 was used to signalize support for the global stats system, which has been removed | |||
// to retain compatibility, we just send a 0, pretending it was simply disabled by the administrator | |||
requestmasterf("server %d %s %d %s 0 %s\n", serverport, *serverip ? serverip : "*", CUR_VERSION, escapestring(limitstring(G(serverdesc), MAXSDESCLEN+1)), escapestring(versionbranch)); | |||
char* advertisedIp; |
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.
Style again: specify the pointer with char* advertisedIp;
to make it consistent with the code below.
} else if (*serverip != '\0') { | ||
advertisedIp = serverip; | ||
} else { | ||
advertisedIp = "*"; |
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.
bn/src/game/auth.h:447:32: warning: ISO C++ forbids converting a string constant to ‘char*’ [-Wwrite-strings]
447 | advertisedIp = "*";
| ^~~
Consider const
-ifying
@@ -32,6 +32,7 @@ out | |||
*.core | |||
*.diff | |||
*.patch | |||
!docker/serverip.patch |
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.
You can remove this now as it does not exist anymore.
See https://github.com/TheAssassin/base/pkgs/container/bn-server for a demo. The images are fully working and optimized for size.
For now, we will only have a moving target tag called
master
(and possibly others for other branches) for public use. Once we create a tag, we'll have a similarly named tag pushed to the registry as well.The
Dockerfile
and the docker-compose config were written from scratch. The remaining scripting comes from https://github.com/TheAssassin/redeclipse-docker/.Note that we do not have any documentation on how to use these. We should document how you can dump the
servinit.cfg
template so as a user, you can see which variables are available.Edit: to run the most basic server locally, use the following command:
> docker run --rm -it -p 28801:28801/udp -p 28802:28802/udp ghcr.io/theassassin/bn-server:master