-
Notifications
You must be signed in to change notification settings - Fork 21
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
Auto Import Database Schema #901
base: main
Are you sure you want to change the base?
Conversation
c648814
to
9b32f6c
Compare
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.
In addition to the changes requested I want to propose/discuss two additional changes:
-
Embed schema files to reduce code related to flag and file handling. Also, this ensures functionality across all contexts without needing to specify the correct schema path.
-
Introduce something like
ErrNoSchema
andErrSchemaMismatch
, and modifyCheckSchema()
accordingly. Build error handling and schema import around these changes. This would enhance code flow and readability, as we now callImportSchema()
followed byCheckSchema()
.
I though about that as well while loosely following this PR. How would this work for package installs? Don't install as file? Both embed and install as a file? Introduce some build flags to control embedding? Like when I was thinking about this, I wasn't sure if this would even simplify things in the big picture (maybe unless the schema is shipped twice, once for manual use in a file, once embedded for automated use). |
I would ship the schema twice and not change anything in the documentation and package installation, as this currently only serves the purpose of moving our container file to this repo. As you said, the embedded file is for automated use and the file on disk is for package/manual use. |
9b32f6c
to
993a62f
Compare
993a62f
to
3b72c2d
Compare
To get rid of docker-icingadb and its additional entry point, the schema import functionality has been implemented directly in Icinga DB. Using the new --database-auto-import command line argument will result in an automatic schema import if no schema is found. The implementation is split between the already existing CheckSchema function and the introduced ImportSchema function. The CheckSchema function is now able to distinguish between the absence of a schema and an incorrect schema version. Both situations return a separate error type. As before, CheckSchema is called in the main function. If the error type now implies the absence of a schema (ErrSchemaNotExists) and the --database-auto-import flag is set, the auto-import is started. The schema import itself is performed in the new ImportSchema function, which loads the schema from a given file and inserts it within a transaction, allowing to rollback in case of an error. Fixes #896.
3b72c2d
to
a2a3751
Compare
At the moment, I am against embedding the schema files because the container still needs the schema update files. So embedding the schema itself while still having to ship the schema updates is an inconsistency, also for the user. If at some point we do have automatic schema updates, then I would argue in favor of embedding everything in the binary, but until we get there, I would leave it the way it is.
I liked this idea and changed the PR in its favor. The initial check logic (schema exists, schema has expected version) went in So I have updated the commit message and the initial post within this PR thread. I have also included two runs with hopefully every possible situations for MySQL/MariaDB and PostgreSQL in the OP. Furthermore, |
Our container doesn't ship schema upgrade files, and I don't think it should unless we have support for automatic schema migrations (which would then be embedded as well anyway). I think you were thinking of the schema upgrade situation in container setups, but this is something where we leave our users alone at the moment as there is no documentation, which of course is not good. But that should rather be a place in the documentation where the schema upgrade files can be downloaded, otherwise you would have to document the path in the container and then users would have to copy the scripts out of the container. |
You are right. I assumed that our container image would contain the schema upgrades as well, but in its current form it only contains a zipped version of the schema. However, for the moment I would still keep it as it is. When eventually adding automatic schema updates, I would argue for embedding the schema file and schema upgrades into the binary, adding Docker (or container) documentation and removing all SQL files from our package builds as well. |
To get rid of docker-icingadb and its additional entry point, the schema import functionality has been implemented directly in Icinga DB. Using the new
--database-auto-import
command line argument will result in an automatic schema import if no schema is found.The implementation is split between the already existing
CheckSchema
function and the introducedImportSchema
function.The
CheckSchema
function is now able to distinguish between the absence of a schema and an incorrect schema version. Both situations return a separate error type.As before,
CheckSchema
is called in themain
function. If the error type now implies the absence of a schema (ErrSchemaNotExists
) and the--database-auto-import
flag is set, the auto-import is started.The schema import itself is performed in the new
ImportSchema
function, which loads the schema from a given file and inserts it within a transaction, allowing to rollback in case of an error.MySQL Run
PostgreSQL Run
Fixes #896.