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

Check mysql structure_load for errors #20569

Merged
merged 1 commit into from Sep 22, 2015

Conversation

Projects
None yet
5 participants
@theSteveMitchell

theSteveMitchell commented Jun 15, 2015

If Mysql is not installed, running rake db:structure:load gives no output, implying success. With this fix, the task will return an error either indicating that mysql does not exist, or that the the process responded with an error.

This is a fix for the mysql_database_tasks and compliments #20468, which fixes postgres_database_tasks

Show outdated Hide outdated activerecord/test/cases/tasks/mysql_rake_test.rb
Kernel.expects(:system).with('mysql', '--execute', %{SET FOREIGN_KEY_CHECKS = 0; SOURCE #{filename}; SET FOREIGN_KEY_CHECKS = 1}, "--database", "test-db")
Kernel.expects(:system)
.with('mysql', '--execute', %{SET FOREIGN_KEY_CHECKS = 0; SOURCE #{filename}; SET FOREIGN_KEY_CHECKS = 1}, "--database", "test-db")
..returns(true)

This comment has been minimized.

@simon2k

simon2k Jun 15, 2015

Could you check it out, if here should be just one dot? :)

@simon2k

simon2k Jun 15, 2015

Could you check it out, if here should be just one dot? :)

@joonas

This comment has been minimized.

Show comment
Hide comment
@joonas

joonas Sep 2, 2015

Having just ran in to this same issue and authored a patch to fix it as well (before looking in the repo and finding this), I'd love to see this get merged 👍

joonas commented Sep 2, 2015

Having just ran in to this same issue and authored a patch to fix it as well (before looking in the repo and finding this), I'd love to see this get merged 👍

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 2, 2015

Member

This looks good to me. r? @senny WDYT?

Member

rafaelfranca commented Sep 2, 2015

This looks good to me. r? @senny WDYT?

Show outdated Hide outdated activerecord/lib/active_record/tasks/mysql_database_tasks.rb
raise "Unable to load structure, is mysql installed?"
when false
raise "Error loading structure, mysql exited with status #{$?.exitstatus}"
end

This comment has been minimized.

@senny

senny Sep 7, 2015

Member

Ca we extract those into a general helper as we did in PostgreSQLDatabaseTasks?

      def run_cmd(cmd, args, action)
        fail run_cmd_error(cmd, args, action) unless Kernel.system(cmd, *args)
      end

      def run_cmd_error(cmd, args, action)
        msg = "failed to execute:\n"
        msg << "#{cmd} #{args.join(' ')}\n\n"
        msg << "Please check the output above for any errors and make sure that `#{cmd}` is installed in your PATH and has proper permissions.\n\n"
        msg
      end
@senny

senny Sep 7, 2015

Member

Ca we extract those into a general helper as we did in PostgreSQLDatabaseTasks?

      def run_cmd(cmd, args, action)
        fail run_cmd_error(cmd, args, action) unless Kernel.system(cmd, *args)
      end

      def run_cmd_error(cmd, args, action)
        msg = "failed to execute:\n"
        msg << "#{cmd} #{args.join(' ')}\n\n"
        msg << "Please check the output above for any errors and make sure that `#{cmd}` is installed in your PATH and has proper permissions.\n\n"
        msg
      end

This comment has been minimized.

@theSteveMitchell

theSteveMitchell Sep 8, 2015

@senny sure we can. What do you think about modifying run_cmd_error to differentiate command not found from a regular failure?

@theSteveMitchell

theSteveMitchell Sep 8, 2015

@senny sure we can. What do you think about modifying run_cmd_error to differentiate command not found from a regular failure?

Show outdated Hide outdated activerecord/test/cases/tasks/mysql_rake_test.rb
Kernel.expects(:system).with("mysqldump", "--result-file", filename, "--no-data", "--routines", "test-db").returns(true)
Kernel.expects(:system)
.with("mysqldump", "--result-file", filename, "--no-data", "--routines", "test-db")
.returns(true)

This comment has been minimized.

@senny

senny Sep 7, 2015

Member

Did anything in this section change or was it just reformatted? If it's just reformatting I'd rather keep it the way it's now. This will keep the patch focused on the actual change, won't mess with git blame and will make it easier to backport if need be.

@senny

senny Sep 7, 2015

Member

Did anything in this section change or was it just reformatted? If it's just reformatting I'd rather keep it the way it's now. This will keep the patch focused on the actual change, won't mess with git blame and will make it easier to backport if need be.

This comment has been minimized.

@theSteveMitchell

theSteveMitchell Sep 8, 2015

OK, I'll remove this change

@theSteveMitchell

theSteveMitchell Sep 8, 2015

OK, I'll remove this change

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Sep 7, 2015

Member

added a few minor comments.

@theSteveMitchell thanks for extending this to MySQL, could you add an entry to the CHANGELOG?

Member

senny commented Sep 7, 2015

added a few minor comments.

@theSteveMitchell thanks for extending this to MySQL, could you add an entry to the CHANGELOG?

@theSteveMitchell

This comment has been minimized.

Show comment
Hide comment
@theSteveMitchell

theSteveMitchell Sep 15, 2015

@senny made the corrections you suggested. Look good?

theSteveMitchell commented Sep 15, 2015

@senny made the corrections you suggested. Look good?

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Sep 17, 2015

Member

@theSteveMitchell thanks for the update.

Code is looking good but I noticed that this patch will print the full commands including passwords. I don't think we should print passwords like this. See this comment for more details on why it's fine in the PostgreSQL adapter.

It would probably be best if we only print a partial command to conceal the password.

/cc @matthewd

Member

senny commented Sep 17, 2015

@theSteveMitchell thanks for the update.

Code is looking good but I noticed that this patch will print the full commands including passwords. I don't think we should print passwords like this. See this comment for more details on why it's fine in the PostgreSQL adapter.

It would probably be best if we only print a partial command to conceal the password.

/cc @matthewd

@theSteveMitchell

This comment has been minimized.

Show comment
Hide comment
@theSteveMitchell

theSteveMitchell Sep 18, 2015

Good point @senny. Will just print the command, not the args. Updated the code.

theSteveMitchell commented Sep 18, 2015

Good point @senny. Will just print the command, not the args. Updated the code.

@senny senny merged commit 4066444 into rails:master Sep 22, 2015

senny added a commit that referenced this pull request Sep 22, 2015

Merge pull request #20569 from theSteveMitchell/master
Check mysql structure_load for errors
@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Sep 22, 2015

Member

@theSteveMitchell thank you 💛

I made some minor modifications in the merge commit (3931cec):

  • Added a changelog entry
  • Changed the message from: failed to execute:\nmysqldumpPlease check to failed to execute: mysqldump\n
  • Removed trailing whitespace
  • Removed a duplicated test when the implementation relied on $?.
Member

senny commented Sep 22, 2015

@theSteveMitchell thank you 💛

I made some minor modifications in the merge commit (3931cec):

  • Added a changelog entry
  • Changed the message from: failed to execute:\nmysqldumpPlease check to failed to execute: mysqldump\n
  • Removed trailing whitespace
  • Removed a duplicated test when the implementation relied on $?.

sgrif added a commit that referenced this pull request Oct 20, 2015

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