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

feature: Added ability to validate database schema #6303

Merged
merged 5 commits into from Apr 5, 2017

Conversation

Projects
None yet
5 participants
@laf
Member

laf commented Mar 30, 2017

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

  • Have you signed the Contributors agreement - please do NOT submit a pull request unless you have (signing the agreement in the same pull request is fine). Your commit message for signing the agreement must appear as per the docs.
  • Have you followed our code guidelines?

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

This is not ready for merging yet as it needs the deploy script finishing and adding to travis.

For users, validate will compare the database schema against a known working one and then provide queries to bring the schema into line. Some things...

  • I've not yet added support for creating missing tables.
  • I've not yet added support for creating missing columns.
  • I have seen an issue with users table where an update to created_at and updated_at still doesn't work as the order needs to be particular which this doesn't cover.
  • I've not done auto fixing but this is possible, for now I've asked users to report the recommended schema changes so we can validate them until we are happy.

When a PR is merged in, travis will rebuild the schema file and if it's changed then it will commit that to the master branch automatically. The only issue with this is if someone pulls in an update in the time it takes for a merge to happen and travis to rebuild the file - that's acceptable to me.

@laf laf added the Blocker 🚫 label Mar 30, 2017

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Mar 30, 2017

Thank you for submitting a PR @laf! We have found the following @murrant, @khobbits and @zarya based on the history of these files to review this PR.

mention-bot commented Mar 30, 2017

Thank you for submitting a PR @laf! We have found the following @murrant, @khobbits and @zarya based on the history of these files to review this PR.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Mar 30, 2017

Auto-Deploy finished, Test PR at http://6303.ci.librenms.org or https://6303.ci.librenms.org

librenms
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Mar 30, 2017

Auto-Deploy finished, Test PR at http://6303.ci.librenms.org or https://6303.ci.librenms.org

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Mar 31, 2017

Member

I like where this headed.

I don't mean to deflate your steam, but this would be a lot better with a DBAL/schema manager like Doctrine.

Maybe this is a needed stepping stone to that. We know that a lot of our user's databases are inconsistent. Mostly because we don't use transactions for our schema updates. (Which in turn requires error-free schema changes)

Member

murrant commented Mar 31, 2017

I like where this headed.

I don't mean to deflate your steam, but this would be a lot better with a DBAL/schema manager like Doctrine.

Maybe this is a needed stepping stone to that. We know that a lot of our user's databases are inconsistent. Mostly because we don't use transactions for our schema updates. (Which in turn requires error-free schema changes)

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Mar 31, 2017

Member

I'm aware the more permanent fix is using a dbal for this but we don't have that currently.

We do have inconsistencies within users db schemas right now and this is done to purely fix this and bridge the gap until we get something better in place. At present we assume users db's are correct so when helping to debug things we can end up on a wild goose chase until we realise it's because a schema update has been missed. This should stop that from happening for now.

It's automatic in it's approach so we don't need to do anything to maintain this at present, I expect it will generate some noise whilst we validate the output it produces but once we're happy we can just enable auto fixing.

It was hardly anytime to put it together so I'm not particular about it being merged in but I do prefer to spend my time enhancing the software rather than bug hunting :)

Member

laf commented Mar 31, 2017

I'm aware the more permanent fix is using a dbal for this but we don't have that currently.

We do have inconsistencies within users db schemas right now and this is done to purely fix this and bridge the gap until we get something better in place. At present we assume users db's are correct so when helping to debug things we can end up on a wild goose chase until we realise it's because a schema update has been missed. This should stop that from happening for now.

It's automatic in it's approach so we don't need to do anything to maintain this at present, I expect it will generate some noise whilst we validate the output it produces but once we're happy we can just enable auto fixing.

It was hardly anytime to put it together so I'm not particular about it being merged in but I do prefer to spend my time enhancing the software rather than bug hunting :)

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Apr 3, 2017

Member

I think json would be fine for this since it is system generated and consumed (no humans in the loop generally)

Should db schema version be included in that file and used for a little extra verification?

Member

murrant commented Apr 3, 2017

I think json would be fine for this since it is system generated and consumed (no humans in the loop generally)

Should db schema version be included in that file and used for a little extra verification?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Apr 3, 2017

Member

Why would you need the schema number? The version of the yaml/json file should match the the schema in that current commit.

Member

laf commented Apr 3, 2017

Why would you need the schema number? The version of the yaml/json file should match the the schema in that current commit.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Apr 3, 2017

Member

With regards to json over yaml. If we use formatted json it's bulky, if we don't it looks a mess and if humans do want to read it it's difficult.

So yaml or formatted json?

Member

laf commented Apr 3, 2017

With regards to json over yaml. If we use formatted json it's bulky, if we don't it looks a mess and if humans do want to read it it's difficult.

So yaml or formatted json?

Check for extra tables and columns
Print 30 items of schema updates, easier to copy paste sql
@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Apr 4, 2017

Member

Never mind about the schema version, we can just check the files, duh.

I compared the outputs:
json_encode was smaller but all on one line
_json_encode was twice the size and a little harder to read than yaml also

So yaml looks the best here. :)

I've added checks for extra tables and columns, is that ok?

Also, I noticed there is no check for the indexes. I think we should check that too if we can.
Creating missing tables and columns does not seem to be handled by the current code. I started to write this, but got hung up on composite keys.

Member

murrant commented Apr 4, 2017

Never mind about the schema version, we can just check the files, duh.

I compared the outputs:
json_encode was smaller but all on one line
_json_encode was twice the size and a little harder to read than yaml also

So yaml looks the best here. :)

I've added checks for extra tables and columns, is that ok?

Also, I noticed there is no check for the indexes. I think we should check that too if we can.
Creating missing tables and columns does not seem to be handled by the current code. I started to write this, but got hung up on composite keys.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Apr 4, 2017

Auto-Deploy finished, Test PR at http://6303.ci.librenms.org or https://6303.ci.librenms.org

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Apr 4, 2017

Member

Yeah I didn't do index checks yet as that needs more work as they need to be parsed.

Member

laf commented Apr 4, 2017

Yeah I didn't do index checks yet as that needs more work as they need to be parsed.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Apr 4, 2017

Member

I'll go through this tonight a little more to see if I can complete the table creation bit. If we don't get indexes tackled, I think we can merge this PR and handle that later.

Member

murrant commented Apr 4, 2017

I'll go through this tonight a little more to see if I can complete the table creation bit. If we don't get indexes tackled, I think we can merge this PR and handle that later.

Now supports detections and suggested fix for:
tables: missing, extra
columns: missing, extra, incorrect
indexes: missing, extra, incorrect
@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Apr 5, 2017

Member

@laf I think this is in decent shape. All that is left is hooking up the travis-ci part.

Also, why the 60 second sleep in build-schema.php? exec calls wait until the command returns right?

Member

murrant commented Apr 5, 2017

@laf I think this is in decent shape. All that is left is hooking up the travis-ci part.

Also, why the 60 second sleep in build-schema.php? exec calls wait until the command returns right?

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Apr 5, 2017

Auto-Deploy finished, Test PR at http://6303.ci.librenms.org or https://6303.ci.librenms.org

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Apr 5, 2017

Member

@murrant I just put it in as a safe guard as locally when testing I had a couple of times where the schema change hadn't applied fully.

Member

laf commented Apr 5, 2017

@murrant I just put it in as a safe guard as locally when testing I had a couple of times where the schema change hadn't applied fully.

librenms
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Apr 5, 2017

Auto-Deploy finished, Test PR at http://6303.ci.librenms.org or https://6303.ci.librenms.org

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Apr 5, 2017

The inspection completed: 3 updated code elements

scrutinizer-notifier commented Apr 5, 2017

The inspection completed: 3 updated code elements

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Apr 5, 2017

Member

This is either going to go well or not - merging :)

Member

laf commented Apr 5, 2017

This is either going to go well or not - merging :)

@laf laf merged commit 03deb64 into librenms:master Apr 5, 2017

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@laf laf deleted the laf:schema-validation branch Apr 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment