fix(DB/Import) Support multiple styles of sql paths in DBUpdater

[michaeldelago]

Changes Proposed:

• Some modules have sql files in data/sql/db-world. Other modules have sql files in sql/world. data/sql/db-world was referenced in the code, meaning automatic imports would only work with some modules.
• This change puts both paths into consideration
• examples
  • mod-transmog uses data/sql
  • mod-eluna uses sql/
  • mod-ah-bot uses sql/
• the skeleton-module use sql/

Issues Addressed:

• Issue described in fix(Docker): Remove db-import container, automatically import module SQL #16155
  • This change also has fixes to use a proper tmpfile for the "create_table" file. This is probably something that can be used in a few other places.

SOURCE:

Tests Performed:

• Tested with docker due to reproducible builds

How to Test the Changes:

1. Download 2 modules that have sql in the different directories

   git clone https://github.com/azerothcore/mod-transmog.git modules/mod-transmog
   git clone https://github.com/azerothcore/mod-item-level-up.git modules/mod-item-level-up

2. make the DBimport a No-op.
   a. Your yq implementation may need a different command. This is community/yq in the arch linux repository

   yq -iY '.services."ac-db-import".command |= "/bin/true"' docker-compose.yml

3. Build and start server

   ./acore.sh docker clean:build && \
   ./acore.sh docker build && \
   ./acore.sh docker start:app

4. As the container runs, observe SQL from both mod-transmog and mod-item-level-up get imported

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.

[heyitsbench]

I would honestly prefer only one path (data/sql/db-world) to work and to just PR modules to use the specified path to make use of the automatic updates, but that's just me.

[michaeldelago]

I would honestly prefer only one path (data/sql/db-world) to work and to just PR modules to use the specified path to make use of the automatic updates, but that's just me.

I don't disagree, but unfortunately (as far as I can tell) there hasn't been a main standardization effort for the sql path. I'd say it's more of a good thing that the vast majority of modules fall into one of 2 paths, rather than more. Without doing the extra work of changing all modules, supporting both of the paths will just make the server easier to get started with and support in the long run.

Regardless of what if multiple are supported or not I think that the naming scheme should be based on the module-skeleton (which is sql/). If we want to use the other scheme, we should change the skeleton first. Writing a script that will submit a PR to every repo to move some directories around won't be hard, but going through all of the PR's and ensuring the module still works properly (not that an issue is likely) will be tedious and time consuming.

[heyitsbench]

Without doing the extra work of changing all modules, supporting both of the paths will just make the server easier to get started with and support in the long run.

Up to maintainers at the end of the day I suppose, but not the route I would go.

but going through all of the PR's and ensuring the module still works properly (not that an issue is likely)

Doubt the module would work properly whether or not the SQL was automated, but an effort to correct the path would at least reveal whether it works or not.

will be tedious and time consuming.

Never stopped me before. 😛 If maintainers want to go the 'one path to rule them all' route, I'd be willing to go through the catalogue and correct any offending modules, though no guarantee of correcting any issues brought along by core updates.

[Nyeriah]

I think this is fine, although maybe ultimately some SQL wasn't intended to be ran automatically

[heyitsbench]

Worth noting that this may also cause issues for existing users that have already applied the SQL for their modules manually. Not all module queries are created with the intention of being rerunnable, so having it be picked up from the automated updater may cause problems.

[AnchyDev]

I think people should just update their modules to use the proper pathing.
If you find a module with incorrect pathing and it is intended to be applied in an ordered series then maybe you could PR.

This change doesn't really negatively affect me however.

[heyitsbench]

Doesn't affect me whatsoever, but I know that support for changes like this may be hard to deal with sometimes.

[FrancescoBorzi]

what's the status of this?

[michaeldelago]

what's the status of this?

@FrancescoBorzi

It didn't seem to be something that was supported by reviewers, which is fine. I agree that ensure that module authors/maintainers fix the path is a good option.

I've had the intent to go through the azerothcore org and try to automatically generate PR's to convert these paths, but I haven't had the time to unfortunately.

Closing for now.