Skip to content
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

MBS-11072: Improve GenerateSQLScripts.pl usability #1688

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

yvanzo
Copy link
Contributor

@yvanzo yvanzo commented Sep 9, 2020

Problem

MBVM-60: Output of setup-amqp-triggers is misleading and incomplete

Solution

MBS-11072:

  • Self-document GenerateSQLScripts with -h option
  • Add an option to specify a limited set of files to be parsed
  • Improve output to log about successfully parsed files

Checklist for author

  • Locally tested with setup-amqp-triggers

Action

  1. Merge MBVM-60: Clean setup-amqp-triggers output musicbrainz-docker#172 when upgrading to the next release of MusicBrainz Server.

Move copyright notice to top POD and update from Git log.
The added option `-c` | `--create-scripts` allows to specify a limited
list of SQL Create files to be parsed to generate scripts. For example:

    GenerateSQLScripts.pl \
        -c CreateFunctions.sql CreateTriggers.sql \
        /tmp/indexer-sql
@yvanzo yvanzo added the QoL Non-urgent quality of life improvements label Sep 9, 2020
@mwiencek
Copy link
Member

mwiencek commented Sep 9, 2020

I'm confused. Why does the Docker setup need to run GenerateSQLScripts.pl for sir when the Drop* scripts for it are already stored in its repo? It even has its own GenerateDropSql.pl which shouldn't have those misleading messages. Am I missing something?

@yvanzo
Copy link
Contributor Author

yvanzo commented Sep 9, 2020

Indeed, it seems that GenerateSQLScripts.pl’s call from SIR’s Makefile predates the script GenerateDropSql.pl which is a derivative simplified version of GenerateSQLScripts.pl.

If I recall correctly, SIR used to call scripts directly from MBS, assuming it was installed in the same system, which is not the case with separate Docker containers.

Also I did not find any other call to GenerateSQLScripts.pl in our repositories even though it seems it may have been used for CAA in the past? The Git history doesn’t really explain the rationale of this script either. Is it solely for use by maintainers during schema change or something like that?

@mwiencek
Copy link
Member

mwiencek commented Sep 14, 2020

Also I did not find any other call to GenerateSQLScripts.pl in our repositories even though it seems it may have been used for CAA in the past? The Git history doesn’t really explain the rationale of this script either. Is it solely for use by maintainers during schema change or something like that?

Yeah, it's for regenerating scripts after schema changes. (The CAA schema is in this repo.) Which is mostly why it only logs when it didn't find something - that could indicate an error, whereas seeing what changes it generated is just git diff / git status.

@reosarevok
Copy link
Member

So should we stop using this file in sir instead?

@mwiencek
Copy link
Member

We use it in musicbrainz-docker, not sir. But musicbrainz-docker should be able to use the scripts in https://github.com/metabrainz/sir/tree/master/sql AFAICT and not have to generate anything. The documentation improvements here make sense at least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QoL Non-urgent quality of life improvements
Projects
None yet
3 participants