feat: ✨ Add new branch to work with Docker #256

gmargriff · 2024-04-16T22:40:29Z

Hi, I'm working in this project's Docker support. Hope anyone can better test it's compatibility with current client. Had to make a few changes in server files so that it's compatible with Linux + works with docker initialization. It would be better to keep this in a separate Docker branch for now. Thanks!

…binaries

exectails · 2024-04-18T22:53:14Z

Thanks for sharing your work. We wouldn't want to merge this into the main branch, but keeping it in a seperate one might be a nice idea. Though there are a couple changes I'm personally not happy with, such as merging all SQL files or removing the console command code entirely. I feel like we'd want to keep as much of the code the same as possible, without major differences between versions/branches.

Syncing last repo changes

Allow usage of command inputs on baremetal Windows OS while keeping disable for incompatible systems (UNIX + Docker and MacOS)

… folder

gmargriff · 2024-04-19T16:50:59Z

Thanks for sharing your work.

Thank you for your project, dedication and for taking the time to review this fork.

We wouldn't want to merge this into the main branch, but keeping it in a seperate one might be a nice idea.

Yeah, I think it's quite a change of scope to be merged straight to master.

Though there are a couple changes I'm personally not happy with, such as merging all SQL files or removing the console command code entirely.

I agree, I did the SQL merge because it's easier to setup the Docker entrypoint with one file, but there was no good reason to remove the original files. The console commands shouldn't have been removed either, so I've added a feel checks to skip incompatible commands on incompatible systems whille keeping it working as intented on Windows baremetal OS.

I feel like we'd want to keep as much of the code the same as possible, without major differences between versions/branches.

Also agreed. I'll try to keep up!

Docker

Sync repositories

Sync branches

exectails · 2024-07-06T22:40:45Z

.env.example

@@ -0,0 +1,11 @@
+# ENVIRONMENT determines the folder where we'll look for binaries.


I believe it would be beneficial to include a brief explanation of what this file is, for anyone who might look inside without knowing about Docker.

exectails · 2024-07-06T22:43:47Z

README.md

@@ -41,23 +35,105 @@ Specifically, some of the major features that are working are as follows:
 - Monster spawns
 - Quests

-Requirements
+


While a guide for the setup via Docker is of course a good idea, I think it's a bit excessive for the readme, which, in my opinion, should be more of a summary for the application as a whole. We also don't include a detailed guide on setting up the client for the same reason. Such information is better suited for the wiki. Or better yet, our underutilized doc folder,

exectails · 2024-07-06T22:46:00Z

Servers/Dockerfile

@@ -0,0 +1,10 @@
+FROM ubuntu:22.04


I believe the name of this folder could be chosen better, as "Servers" has a rather general purpose ring to it, that seems misleading, seeing how it's only used for Docker.

exectails · 2024-07-06T22:48:14Z

src/BarracksServer/BarracksServer.cs

-			ConsoleUtil.LoadingTitle();
+
+			// Skip the following command on incompatible systems
+			if (Environment.OSVersion.Platform == PlatformID.Win32NT)


Is this actually an issue? Does it throw an exception if ran through Docker or something? If so, we'd probably rather want to make this a general check inside the LoadingTitle method, where we make sure there's actually a window that we can change the title of.

exectails · 2024-07-06T22:50:27Z

src/BarracksServer/BarracksServer.cs

-			new BarracksConsoleCommands().Wait();
+			// If running on Windows system, enable console inputs
+			// otherwise, start an event loop to keep server running until stopped
+			if (Environment.OSVersion.Platform == PlatformID.Win32NT)


I think the check for Windows is misguided here, because I could also be running Melia under Linux or MacOS for example, without using Docker. Instead, a check for an interactive console would seem more appropriate.

exectails · 2024-07-06T22:52:16Z

src/BarracksServer/BarracksServer.cs

-				{
-					if (serverUpdateMessage.ServerType == ServerType.Zone)
-						_zoneServerNames[sender] = serverUpdateMessage.ServerId;
+					{


Your auto-formatter seems to have intended a the switch cases, which is somewhat undesirable. Please make sure it's operating according to our coding conventions.

exectails · 2024-07-06T22:55:14Z

src/Shared/Configuration/Files/Web.cs

@@ -19,7 +19,7 @@ public void Load(string filePath)
 		{
 			this.Include(filePath);

-			this.PhpCgiFilePath = this.GetString("php_cgi_bin", Path.Combine("user", "tools", "php", "php-cgi.exe"));
+			this.PhpCgiFilePath = this.GetString("php_cgi_bin", "/usr/bin/php-cgi");


Instead of changing the default values of the configs, wouldn't it be cleaner to just change the user conf values during the Docker setup? It wouldn't have to mess with the system conf at all.

exectails · 2024-07-06T22:58:34Z

start

@@ -0,0 +1,56 @@
+#!/bin/sh
+killall -9 BarracksServer


It seems like this bash script could more or less serve the purpose of the batch scripts we have, which is good for Linux users, but it probably shouldn't kill all servers on start for that to work, because you would then not be able to spin up new servers, or restart one for example, without killing all of them.

exectails · 2024-07-06T23:00:07Z

stop

@@ -0,0 +1,7 @@
+#!/bin/sh
+clear


Might just be me, but I think I'd be a bit annoyed if the stop script cleared my screen every time. What if there was still something I needed?

exectails · 2024-07-06T23:02:21Z

system/conf/database.conf

 user     : root
-pass     : 
+pass     : 123456


Same as with the PHP options, you don't need to modify the system configs if you just create the respective user conf files.

exectails · 2024-07-07T12:03:30Z

I believe if those couple things are improved upon, this is basically fine to be merged into a branch, or even master, if it manages to be general purpose enough.

y4my4my4m · 2024-08-09T07:02:05Z

fyi, under windows, i had to add vim to the packges of the dockerfile and do :set ff=unix to the start file because it was copying the file to the docker with non-unix CLRF file ending

y4my4my4m · 2024-08-09T10:29:45Z

gmargriff#4
I made a PR to @gmargriff 's repo (because im using the docker version for convenience) but thought the docker ver should get merged in the main repo too. If the docker doesnt get merged, maybe you'd still like to add my changes.

More "get all items of type X" commands

exectails

Thanks for the continued work on this PR. I'd like to merge it in the near future, but unfortunately there are still some issues and potential improvements, as well as comments that weren't addressed yet.

exectails · 2024-09-02T19:17:16Z

src/Shared/Configuration/Files/Web.cs

@@ -17,8 +18,12 @@ public class WebConfFile : ConfFile
 		public void Load(string filePath)
 		{
 			this.Include(filePath);
-
-			this.PhpCgiFilePath = this.GetString("php_cgi_bin", Path.Combine("user", "tools", "php", "php-cgi.exe"));
+            if (Environment.OSVersion.Platform == PlatformID.Win32NT)


This won't really change anything, because the configuration value is read from the conf file regardless of the operating system. The only way you will get the usr bin path from this is if the conf option is missing.

exectails · 2024-09-02T19:18:01Z

src/ZoneServer/Commands/ChatCommands.Handlers.cs

@@ -63,7 +63,21 @@ public ChatCommands()
 			this.Add("item", "<item id> [amount]", "Spawns item.", this.HandleItem);
 			this.Add("silver", "<modifier>", "Spawns silver.", this.HandleSilver);
 			this.Add("spawn", "<monster id|class name> [amount=1] ['ai'=BasicMonster] ['tendency'=peaceful] ['hp'=amount]", "Spawns monster.", this.HandleSpawn);
-			this.Add("madhatter", "", "Spawns all headgears.", this.HandleGetAllHats);
+
+			this.Add("madhatter", "", "Spawns all hair accessories.", this.HandleGetAllHairAccessories);


This isn't related to the docker PR, is it? 🤔

exectails · 2024-09-02T19:19:27Z

src/ZoneServer/ZoneServer.cs

-			ConsoleUtil.LoadingTitle();
+
+			// Skip the following command on incompatible systems
+			if (Environment.OSVersion.Platform == PlatformID.Win32NT)


We since fixed the problem with LoadingTitle under Linux, and this should now work without any exceptions made in the code.

exectails · 2024-09-02T19:23:21Z

src/ZoneServer/ZoneServer.cs

+
+			// If running on Windows system, enable console inputs
+			// otherwise, start an event loop to keep server running until stopped
+			if (Environment.OSVersion.Platform == PlatformID.Win32NT)


This seems like a suboptimal thing to do, because if I'm running the server in a console under Linux, I might want console commands. In the same vein, I might run the server without a console on Windows. It seems to me like a check for an interactive environment would be more appropriate.

We actually have a property for that on ConsoleUtil, "IsUserInteractive", though I'll admit this property hasn't been used or confirmed to still be working in a while.

exectails · 2024-09-02T19:25:57Z

system/conf/web.conf

@@ -5,6 +5,6 @@
 // Path to the PHP CGI executable
 // If the file doesn't exist, and the configured path is in user/tools/,
 // PHP is downloaded automatically on Windows.
-php_cgi_bin: user/tools/php/php-cgi.exe
+php_cgi_exe: /usr/bin/php-cgi


You seem to have reverted a recent change here, where the name of the option was fixed. It was supposed to be _bin. Additionally, it would be better if, instead of changing default values in conf files in the system folder, you would dynamically make your changes in the user folder for Docker.

exectails · 2024-09-02T19:27:12Z

user/conf/database-example-for-docker.conf

@@ -0,0 +1,8 @@
+// Melia


Do we need an example db conf file for Docker? Will this not be created during the setup process?

exectails · 2024-09-02T19:28:38Z

README.md

@@ -41,7 +35,116 @@ Specifically, some of the major features that are working are as follows:
 - Monster spawns
 - Quests

-Requirements
+Installation with Docker


I'd prefer you revert the changes to the readme, which is supposed to be both concise and general purpose. Long explanations about the setup are better suited for the docs folder.

exectails · 2024-09-02T19:29:56Z

sql/updates/update_2024-08-11_1.sql

@@ -1,18 +1,17 @@
-CREATE TABLE `character_etc_properties` (
-  `propertyId` bigint(20) NOT NULL,
+CREATE TABLE IF NOT EXISTS `character_etc_properties` (


Can you tell me what this change is about? I feel like we'd want this to fail if the table already exists, because that's an indicator for something not being right.

gmargriff · 2024-09-02T22:46:40Z

Hey, @exectails and @y4my4my4m , thanks for your work in this PR, I've done just enough changes to make autosync available again on Github, but I'm not directly working on this right now since my PC has been down since may/june, just changed the PR to draft to avoid confusion. Through my phone I've been just keeping the repo synced. I've read through the comments here and it all seems reasonable, as soon as I can get back I'll be working on this. Thanks again!

gmargriff added 6 commits April 16, 2024 19:26

feat: ✨ Add new branch to work with Docker

658fb2a

fix: 🐛 Missing SQL folder when starting server

d9d3a75

fix: 🐛 Changing SQL queries due to capital letters conflict

2a86353

feat: 🎨 Add .env file for configuring docker-compose

f7dd52c

docs: 📝 Improve documentation on docker and client setup

cfa2971

fix: 🐛 Use ENVIRONMENT variable from .env file to run correct server …

1f2e059

…binaries

gmargriff added 4 commits April 19, 2024 13:01

Merge branch 'master' into docker

751c0fb

Syncing last repo changes

fix: ⚡ Re-enable console commands on Windows systems

681cdd0

Allow usage of command inputs on baremetal Windows OS while keeping disable for incompatible systems (UNIX + Docker and MacOS)

revert: 🗃️ Revert removal of SQL files from master branch

840cfb7

refactor: ♻️ Necessary changes in Merged SQL files to keep SQL entire…

06a6f25

… folder

gmargriff and others added 11 commits April 23, 2024 18:15

fix: ⚡ Make sure database started before starting servers

2bb0a58

docs: 📝 Improve documentation ordering for branch specifics

cc5879c

Merge pull request #1 from gmargriff/docker

68eec32

Docker

Merge pull request #2 from NoCode-NoLife/master

c55cd66

Sync repositories

Merge pull request #3 from gmargriff/docker

b724216

Sync branches

Merge branch 'NoCode-NoLife:master' into docker

dc8de12

Merge branch 'NoCode-NoLife:master' into master

5cedc96

Merge branch 'NoCode-NoLife:master' into docker

a7b736b

Merge branch 'NoCode-NoLife:master' into master

2d3b785

Merge branch 'NoCode-NoLife:master' into master

fbfd852

Merge branch 'NoCode-NoLife:master' into docker

47660fe

exectails reviewed Jul 6, 2024

View reviewed changes

gmargriff added 4 commits July 25, 2024 09:57

Merge branch 'NoCode-NoLife:master' into docker

35323dc

Merge branch 'NoCode-NoLife:master' into master

c9de2e7

Merge branch 'NoCode-NoLife:master' into master

7ffd8b3

Merge branch 'NoCode-NoLife:master' into docker

b475db6

more commands

e45283e

y4my4my4m added 8 commits August 10, 2024 20:25

Update ChatCommands.Handlers.cs

bf27246

Update ChatCommands.Handlers.cs

54b0d11

Update commands.conf

90b9773

Merge remote-tracking branch 'y4my4m/master' into new-branch

6b5d9f5

update

bddca91

Merge branch 'master' of github.com:NoCode-NoLife/melia into new-branch

13f3487

temp fix

875c132

git ignore all again

e93bcfe

y4my4my4m mentioned this pull request Aug 13, 2024

Docker fix #297

Closed

gmargriff and others added 4 commits August 29, 2024 16:12

Merge pull request #4 from y4my4my4m/master

b2b25da

More "get all items of type X" commands

Merge branch 'master' into docker

2a8ff97

Merge branch 'master' into docker

a382399

fix mergeability

81d7965

exectails requested changes Sep 2, 2024

View reviewed changes

Merge branch 'NoCode-NoLife:master' into docker

8a5a120

gmargriff marked this pull request as draft September 2, 2024 22:38

gmargriff added 10 commits September 3, 2024 19:28

Merge branch 'NoCode-NoLife:master' into docker

6e298ca

Merge branch 'NoCode-NoLife:master' into docker

ce4fc82

Merge branch 'NoCode-NoLife:master' into docker

b5c0c37

Merge branch 'NoCode-NoLife:master' into docker

50edd51

Merge branch 'NoCode-NoLife:master' into docker

a683071

Merge branch 'NoCode-NoLife:master' into docker

eef7545

Merge branch 'NoCode-NoLife:master' into docker

29a7cd1

Merge branch 'NoCode-NoLife:master' into docker

a196a76

Merge branch 'NoCode-NoLife:master' into docker

43b9145

Merge branch 'NoCode-NoLife:master' into docker

4c66caa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ✨ Add new branch to work with Docker #256

feat: ✨ Add new branch to work with Docker #256

gmargriff commented Apr 16, 2024

exectails commented Apr 18, 2024

gmargriff commented Apr 19, 2024

exectails Jul 6, 2024

exectails Jul 6, 2024

exectails Jul 6, 2024

exectails Jul 6, 2024

exectails Jul 6, 2024

exectails Jul 6, 2024

exectails Jul 6, 2024

exectails Jul 6, 2024

exectails Jul 6, 2024 •

edited

Loading

exectails Jul 6, 2024

exectails commented Jul 7, 2024

y4my4my4m commented Aug 9, 2024

y4my4my4m commented Aug 9, 2024 •

edited

Loading

exectails left a comment

exectails Sep 2, 2024

exectails Sep 2, 2024

exectails Sep 2, 2024

exectails Sep 2, 2024

exectails Sep 2, 2024

exectails Sep 2, 2024

exectails Sep 2, 2024

exectails Sep 2, 2024

gmargriff commented Sep 2, 2024

		@@ -0,0 +1,11 @@
		# ENVIRONMENT determines the folder where we'll look for binaries.

		@@ -0,0 +1,7 @@
		#!/bin/sh
		clear

feat: ✨ Add new branch to work with Docker #256

Are you sure you want to change the base?

feat: ✨ Add new branch to work with Docker #256

Conversation

gmargriff commented Apr 16, 2024

exectails commented Apr 18, 2024

gmargriff commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exectails Jul 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exectails commented Jul 7, 2024

y4my4my4m commented Aug 9, 2024

y4my4my4m commented Aug 9, 2024 • edited Loading

exectails left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmargriff commented Sep 2, 2024

exectails Jul 6, 2024 •

edited

Loading

y4my4my4m commented Aug 9, 2024 •

edited

Loading