azerothcore-wotlk icon indicating copy to clipboard operation
azerothcore-wotlk copied to clipboard

chore. Remove error warning on worldserver

Open pangolp opened this issue 1 year ago • 5 comments

Changes Proposed:

This PR proposes changes to:

logger

  • Error while configuring Appender GM in Logger commands.gm. Appender does not exist
  • [x] worldserver.conf

Issues Addressed:

  • Closes

SOURCE:

The changes have been validated through:

Tests Performed:

This PR has been:

  • [x] Tested in-game by the author.

How to Test the Changes:

  1. Having commented on this line, I don't know if the logs are generated or not, because it depends on the options below, but in the worldserver at the top, a message appears with an error. Let it continue, but this message should not appear.

Known Issues and TODO List:

How to Test AzerothCore PRs

When a PR is ready to be tested, it will be marked as [WAITING TO BE TESTED].

You can help by testing PRs and writing your feedback here on the PR's page on GitHub. Follow the instructions here:

http://www.azerothcore.org/wiki/How-to-test-a-PR

REMEMBER: when testing a PR that changes something generic (i.e. a part of code that handles more than one specific thing), the tester should not only check that the PR does its job (e.g. fixing spell XXX) but especially check that the PR does not cause any regression (i.e. introducing new bugs).

For example: if a PR fixes spell X by changing a part of code that handles spells X, Y, and Z, we should not only test X, but we should test Y and Z as well.

pangolp avatar Feb 12 '24 23:02 pangolp

Comment out the logger instead. It is bad to have this appender open by default

Kitzunu avatar Feb 13 '24 12:02 Kitzunu

Comment out the logger instead. It is bad to have this appender open by default

Could you tell me which file I should search in? Thank you.

pangolp avatar Feb 13 '24 13:02 pangolp

worldserver.conf.dist https://github.com/azerothcore/azerothcore-wotlk/blob/b6ee4f49cfbfc907b91fab446de7d207a2bff8bc/src/server/apps/worldserver/worldserver.conf.dist#L670

Kitzunu avatar Feb 14 '24 14:02 Kitzunu

worldserver.conf.dist https://github.com/azerothcore/azerothcore-wotlk/blob/b6ee4f49cfbfc907b91fab446de7d207a2bff8bc/src/server/apps/worldserver/worldserver.conf.dist#L670

Thanks.

pangolp avatar Feb 14 '24 19:02 pangolp

This pull request is ready to be tested, although it is not urgent. It's just to avoid that message that usually appears at the top of the worldserver when powering on.

pangolp avatar Feb 16 '24 12:02 pangolp

I just tested this PR.

It does indeed remove the error message on server start BUT it also disables all debug messages when GM commands are beeing used.

So right now I can't approve this PR, as imo it's just a workaround and does not fix the "real" issue causing the error message.

sudlud avatar Mar 09 '24 04:03 sudlud

We close the pull request then. Thanks for the test. It was open for quite some time. I would prefer that later someone corrects the error thoroughly as you mention. Greetings.

pangolp avatar Mar 09 '24 18:03 pangolp

Alright so I've taken another look at this after a discussion with @pangolp

The "root cause" for the error message is the commit 12456b54 by @Kitzunu.

In this commit he disabled the Appender.GM without also disabling Logger.commands.gm, which results in the observed error message.

As of the hint provided by @pangolp, it seems that these two options can only exist together.

Therefore this PR indeed seems valid to fix this issue, as the intend of @Kitzunu in 12456b54 seems to have been to disable GM command debug messages by default and this PR finalizes this intention by fixing the error message.

If the changes in the mentioned commit were made by accident, this PR should instead uncomment Appender.GM to properly enable GM command debug messages and also get rid of the error.

In the end right now this is a lose-lose situation as no feedback on this was provided, now I took a look, maybe couldn't grasp the whole situation on the first attempt, the PR author is disappointed, deletes the PR and (again) in the long run we might lose another contributor.

sudlud avatar Mar 09 '24 19:03 sudlud

Alright so I've taken another look at this after a discussion with @pangolp

The "root cause" for the error message is the commit 12456b5 by @Kitzunu.

In this commit he disabled the Appender.GM without also disabling Logger.commands.gm, which results in the observed error message.

As of the hint provided by @pangolp, it seems that these two options can only exist together.

Therefore this PR indeed seems valid to fix this issue, as the intend of @Kitzunu in 12456b5 seems to have been to disable GM command debug messages by default and this PR finalizes this intention by fixing the error message.

If the changes in the mentioned commit were made by accident, this PR should instead uncomment Appender.GM to properly enable GM command debug messages and also get rid of the error.

In the end right now this is a lose-lose situation as no feedback on this was provided, now I took a look, maybe couldn't grasp the whole situation on the first attempt, the PR author is disappointed, deletes the PR and (again) in the long run we might lose another contributor.

It's not your fault that you may have interpreted things wrong, possibly I explained them wrong. Instead of closing it, I should have explained or changed the title as you suggested in the message, so don't worry. Thank you for having done the tests, and after having read my explanation, continue testing the comments I made about it.

pangolp avatar Mar 09 '24 20:03 pangolp

I reopen the pull request then.

pangolp avatar Mar 09 '24 20:03 pangolp

@sudlud In summary, according to your tests, should we then have both lines commented or just having this one is enough?

pangolp avatar Mar 09 '24 20:03 pangolp

This PR finalizes the intent of commit 12456b5 to disable gm command dbg messages by default and also disables the error message on startup.

But right now the dbg messages are being printed out in worldserver console together with an error on server start.

So in the end we need the maintainers' decision now:

  • should gm command dbg messages in worldserver console be enabled by default?

Yes - this PR can be merged as-is No - this PR needs an update

sudlud avatar Mar 09 '24 21:03 sudlud

This PR finalizes the intent of commit 12456b5 to disable gm command dbg messages by default.

But right now the dbg messages are being printed out in worldserver console together with an error on server start.

So in the end we need the maintainers' decision now:

  • should gm command dbg messages in worldserver console be enabled by default?

Yes - this PR can be merged as-is No - this PR needs an update

I would disable messages by default, since they are very annoying, and I would let each project decide whether it wants to enable them or not. At most, information can be added to the wiki so that each community can correctly configure the logs of its worldserver.conf and authserver.conf, because the vast majority of them are left by default.

pangolp avatar Mar 09 '24 21:03 pangolp