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

Add a repair step to delete old tables #14638

Merged
merged 2 commits into from
Mar 3, 2015

Conversation

nickvergessen
Copy link
Contributor

@nickvergessen nickvergessen added this to the 8.1-current milestone Mar 2, 2015
@ghost
Copy link

ghost commented Mar 2, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/10114/
Test PASSed.

@MorrisJobke
Copy link
Contributor

Works 👍

@karlitschek
Copy link
Contributor

👍 nice

@PVince81
Copy link
Contributor

PVince81 commented Mar 2, 2015

Can you write a unit test ?
The test could create one of those tables and test the deletion logic for:

  1. an existing table
  2. a non-existing table

I know the code looks straightforward, but who knows what can happen with Oracle...

$this->emit('\OC\Repair', 'info', [
sprintf('Table %s has been deleted', $tableName)
]);
$this->connection->dropTable($tableName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dropTable could throw an exception which we better catch and log and do not fall apart

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc block does not say something like that 🙈
And also if that thing fails, it has to be due to broken DB credentials, and then other things will bring that to the users attention?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also if that thing fails, it has to be due to broken DB credentials,

will 💣 earlier

@DeepDiver1975
Copy link
Member

Can you write a unit test ?

@nickvergessen yes - this is required - THX

@nickvergessen
Copy link
Contributor Author

Test added

@scrutinizer-notifier
Copy link

The inspection completed: 6 new issues, 9 updated code elements

@ghost
Copy link

ghost commented Mar 3, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/10162/
Test PASSed.

@DeepDiver1975
Copy link
Member

👍

DeepDiver1975 added a commit that referenced this pull request Mar 3, 2015
…ld-tables

Add a repair step to delete old tables
@DeepDiver1975 DeepDiver1975 merged commit e30ca81 into master Mar 3, 2015
@DeepDiver1975 DeepDiver1975 deleted the issue/14538-repairstep-drop-old-tables branch March 3, 2015 12:37
@lock lock bot locked as resolved and limited conversation to collaborators Aug 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Repair step to drop old database tables
6 participants