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

Attempt to fix repeated sql issue we come across #7123

Merged
merged 2 commits into from Aug 14, 2017

Conversation

Projects
None yet
3 participants
@laf
Member

laf commented Aug 4, 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.

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 a repeat offender. We have to attempt to drop the primary key first for those who already have it as you can't attempt to add it back in if it exists, generates an error which means it will be flagged in travis, web ui and console installs as such.

@laf laf added the Schema label Aug 4, 2017

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Aug 7, 2017

Member

Might work... But any idea of the root cause?

Member

murrant commented Aug 7, 2017

Might work... But any idea of the root cause?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Aug 7, 2017

Member

It seems to work ok. At the end of the day it's only the queries people are running manually with the addition of dropping the primary key to stop errors being flagged.

As for the root cause. Difficult to say but we've now made so many changes to older schema files to try and fix the install process that I expect we catch people between two states now, one being that they've already run an older pre-modified schema change and then the second trying to run a later schema change which also fixed what we modified in older schemas but they weren't compatible.

Repeat occurrences of posts to the schema thread on community should be wrapped up and fixed. It's just noise we don't need.

Member

laf commented Aug 7, 2017

It seems to work ok. At the end of the day it's only the queries people are running manually with the addition of dropping the primary key to stop errors being flagged.

As for the root cause. Difficult to say but we've now made so many changes to older schema files to try and fix the install process that I expect we catch people between two states now, one being that they've already run an older pre-modified schema change and then the second trying to run a later schema change which also fixed what we modified in older schemas but they weren't compatible.

Repeat occurrences of posts to the schema thread on community should be wrapped up and fixed. It's just noise we don't need.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Aug 10, 2017

Member

Agreed, just want to make sure we still don't have issues with new installs. If it is people that have borked schema from schema updates that have since been fixed, great :)

Member

murrant commented Aug 10, 2017

Agreed, just want to make sure we still don't have issues with new installs. If it is people that have borked schema from schema updates that have since been fixed, great :)

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Aug 11, 2017

Member

I don't think anyone has a true idea of why we see the differences however....

  1. We merge this and it fixes the issue and reduces the number of reports
  2. We merge this and it doesn't fix it and we continue to get reports.
  3. We don't merge and guarantee to get reports and have people manually fix them

I prefer # 1 and hope we don't see # 2

Member

laf commented Aug 11, 2017

I don't think anyone has a true idea of why we see the differences however....

  1. We merge this and it fixes the issue and reduces the number of reports
  2. We merge this and it doesn't fix it and we continue to get reports.
  3. We don't merge and guarantee to get reports and have people manually fix them

I prefer # 1 and hope we don't see # 2

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Aug 14, 2017

The inspection completed: No new issues

scrutinizer-notifier commented Aug 14, 2017

The inspection completed: No new issues

@laf laf merged commit 7b97e0e into librenms:master Aug 14, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

lock bot commented May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 17, 2018

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