Overhaul database package

Overhaul database package

- Remove SQLite
- Centralize/unify database usage, add lots of config helper functions
- Add setup script
- Add buildbot_migratedb

BUG=skia:

Committed: https://skia.googlesource.com/buildbot/+/bee4e6a84182c691f7ea2c984d54d2ece0230741

[borenet, 2014-12-16 18:42:04]

borenet@google.com changed reviewers:
+ jcgregorio@google.com, stephana@google.com

[borenet, 2014-12-16 18:42:05]

https://codereview.chromium.org/813443002/diff/1/golden/go/db/db.go
File golden/go/db/db.go (right):

https://codereview.chromium.org/813443002/diff/1/golden/go/db/db.go#newcode23
golden/go/db/db.go:23: DB_CONN_TMPL = "%s:%s@tcp(%s:%s)/%s?parseTime=true"
I'd like to further unify this with the database package, since there's some
duplicated logic, but because of the password override it becomes a little
complicated, and this change is already pretty disruptive.

[jcgregorio, 2014-12-16 20:08:39]

On 2014/12/16 18:42:05, borenet wrote:
> https://codereview.chromium.org/813443002/diff/1/golden/go/db/db.go
> File golden/go/db/db.go (right):
> 
> https://codereview.chromium.org/813443002/diff/1/golden/go/db/db.go#newcode23
> golden/go/db/db.go:23: DB_CONN_TMPL = "%s:%s@tcp(%s:%s)/%s?parseTime=true"
> I'd like to further unify this with the database package, since there's some
> duplicated logic, but because of the password override it becomes a little
> complicated, and this change is already pretty disruptive.

Can you add a bash script to install a local mysql instance with the appropriate
user and passwd?

[jcgregorio, 2014-12-16 20:09:24]

https://codereview.chromium.org/813443002/diff/1/golden/go/db/db.go
File golden/go/db/db.go (right):

https://codereview.chromium.org/813443002/diff/1/golden/go/db/db.go#newcode23
golden/go/db/db.go:23: DB_CONN_TMPL = "%s:%s@tcp(%s:%s)/%s?parseTime=true"
On 2014/12/16 18:42:05, borenet wrote:
> I'd like to further unify this with the database package, since there's some
> duplicated logic, but because of the password override it becomes a little
> complicated, and this change is already pretty disruptive.

Yeah, +1 for doing that in another CL.

[borenet, 2014-12-16 20:24:18]

Added script for db setup.

https://codereview.chromium.org/813443002/diff/20001/go/database/setup_test_db
File go/database/setup_test_db (right):

https://codereview.chromium.org/813443002/diff/20001/go/database/setup_test_d...
go/database/setup_test_db:1: #!/bin/bash
Should this run automatically?

[stephana, 2014-12-17 13:13:38]

https://codereview.chromium.org/813443002/diff/20001/go/database/setup_test_db
File go/database/setup_test_db (right):

https://codereview.chromium.org/813443002/diff/20001/go/database/setup_test_d...
go/database/setup_test_db:1: #!/bin/bash
On 2014/12/16 20:24:18, borenet wrote:
> Should this run automatically?

IMO it's fine to run it manually, but there needs to be a README somewhere that
describes what's necessary to set up the database. 

If the database doesn't exist the tests should fail and either point to the
README file or the script.

[borenet, 2014-12-17 14:55:14]

https://codereview.chromium.org/813443002/diff/20001/go/database/setup_test_db
File go/database/setup_test_db (right):

https://codereview.chromium.org/813443002/diff/20001/go/database/setup_test_d...
go/database/setup_test_db:1: #!/bin/bash
On 2014/12/17 13:13:38, stephana wrote:
> On 2014/12/16 20:24:18, borenet wrote:
> > Should this run automatically?
> 
> IMO it's fine to run it manually, but there needs to be a README somewhere
that
> describes what's necessary to set up the database. 
> 
> If the database doesn't exist the tests should fail and either point to the
> README file or the script. 

Added a README.  I'm not sure how to make the error message point back to the
script, since the error would be encountered somewhere down the line when
someone runs sql.Open.  AFAICT there's not an easy way to inject the message at
that point.

[stephana, 2014-12-17 15:28:49]

Agreed including a reference into an error message is not work the effort.

https://codereview.chromium.org/813443002/diff/40001/go/database/README
File go/database/README (right):

https://codereview.chromium.org/813443002/diff/40001/go/database/README#newcode6
go/database/README:6: convenience in setting up this test database and user.
Without going beyond the scope of this CL, I think this should go into the first
version a README in the root of the repo as a section. i.e.

Database Setup For Testing
==========================

<your description>


We want anybody who checks it out to be able to go through your instructions and
be able to get going fast.

[borenet, 2014-12-17 15:39:17]

https://codereview.chromium.org/813443002/diff/40001/go/database/README
File go/database/README (right):

https://codereview.chromium.org/813443002/diff/40001/go/database/README#newcode6
go/database/README:6: convenience in setting up this test database and user.
On 2014/12/17 15:28:49, stephana wrote:
> Without going beyond the scope of this CL, I think this should go into the
first
> version a README in the root of the repo as a section. i.e.
> 
> Database Setup For Testing
> ==========================
> 
> <your description>
> 
> 
> We want anybody who checks it out to be able to go through your instructions
and
> be able to get going fast. 

Done.

[tfarina, 2014-12-17 18:49:31]

https://codereview.chromium.org/813443002/diff/60001/go/database/setup_test_d...
File go/database/setup_test_db.sql (right):

https://codereview.chromium.org/813443002/diff/60001/go/database/setup_test_d...
go/database/setup_test_db.sql:3: GRANT SELECT,INSERT,UPDATE,DELETE,CREATE,DROP
ON sk_testing.* TO 'test_user'@'localhost';
where is the command for creating test_user?

https://codereview.chromium.org/813443002/diff/60001/go/database/setup_test_d...
go/database/setup_test_db.sql:3: GRANT SELECT,INSERT,UPDATE,DELETE,CREATE,DROP
ON sk_testing.* TO 'test_user'@'localhost';
I have something in my playground repo:
https://github.com/tfarina/rsc/blob/master/python/webpy/todo/createdb.sh

[borenet, 2014-12-17 21:02:21]

Okay, I think I have the tests fixed at patch set 5.

https://codereview.chromium.org/813443002/diff/60001/go/database/setup_test_d...
File go/database/setup_test_db.sql (right):

https://codereview.chromium.org/813443002/diff/60001/go/database/setup_test_d...
go/database/setup_test_db.sql:3: GRANT SELECT,INSERT,UPDATE,DELETE,CREATE,DROP
ON sk_testing.* TO 'test_user'@'localhost';
On 2014/12/17 18:49:31, tfarina wrote:
> where is the command for creating test_user?

It's created automatically in the GRANT command if it doesn't exist.

https://codereview.chromium.org/813443002/diff/80001/go/buildbot/db_setup.go
File go/buildbot/db_setup.go (right):

https://codereview.chromium.org/813443002/diff/80001/go/buildbot/db_setup.go#...
go/buildbot/db_setup.go:37: db, err := sqlx.Open("mysql", conf.MySQLString)
Removed the automatic version updating.  I haven't replaced it with a standalone
migratedb program yet.

[borenet, 2014-12-18 14:03:44]

https://codereview.chromium.org/813443002/diff/80001/go/buildbot/db_setup.go
File go/buildbot/db_setup.go (right):

https://codereview.chromium.org/813443002/diff/80001/go/buildbot/db_setup.go#...
go/buildbot/db_setup.go:37: db, err := sqlx.Open("mysql", conf.MySQLString)
On 2014/12/17 21:02:21, borenet wrote:
> Removed the automatic version updating.  I haven't replaced it with a
standalone
> migratedb program yet.

Added in patch set 6.

[jcgregorio, 2014-12-18 15:21:13]

https://codereview.chromium.org/813443002/diff/100001/go/database/setup_test_db
File go/database/setup_test_db (right):

https://codereview.chromium.org/813443002/diff/100001/go/database/setup_test_...
go/database/setup_test_db:1: #!/bin/bash
File wasn't executable when I patched this CL in, is that just an artifact of
using 'patch'?

https://codereview.chromium.org/813443002/diff/100001/go/database/setup_test_...
go/database/setup_test_db:3: 
set -x -e

[borenet, 2014-12-18 19:12:43]

Okay, ready for another round of review at patch set 10.

https://codereview.chromium.org/813443002/diff/180001/ct/go/util/gs_test.go
File ct/go/util/gs_test.go (right):

https://codereview.chromium.org/813443002/diff/180001/ct/go/util/gs_test.go#n...
ct/go/util/gs_test.go:16: t.Skip("WARNING: Skipping this test due to
https://code.google.com/p/skia/issues/detail?id=3261")
Sorry to do this, but these failures are totally obscuring test output.

https://codereview.chromium.org/813443002/diff/180001/go/buildbot/db.go
File go/buildbot/db.go (right):

https://codereview.chromium.org/813443002/diff/180001/go/buildbot/db.go#newco...
go/buildbot/db.go:95: func (b Build) ReplaceIntoDB() error {
This increases stability. We occasionally get deadlocks while trying to insert
steps.

[rmistry, 2014-12-18 19:18:12]

rmistry@google.com changed reviewers:
+ rmistry@google.com

[rmistry, 2014-12-18 19:18:13]

https://codereview.chromium.org/813443002/diff/180001/ct/go/util/gs_test.go
File ct/go/util/gs_test.go (right):

https://codereview.chromium.org/813443002/diff/180001/ct/go/util/gs_test.go#n...
ct/go/util/gs_test.go:16: t.Skip("WARNING: Skipping this test due to
https://code.google.com/p/skia/issues/detail?id=3261")
On 2014/12/18 19:12:42, borenet wrote:
> Sorry to do this, but these failures are totally obscuring test output.

Joe, what is the right way to deal with this? we had decided on skipping it if
it is a short test in:
https://codereview.chromium.org/779633003/diff/60001/ct/go/util/gs_test.go#ne...

[borenet, 2014-12-18 19:19:23]

https://codereview.chromium.org/813443002/diff/180001/ct/go/util/gs_test.go
File ct/go/util/gs_test.go (right):

https://codereview.chromium.org/813443002/diff/180001/ct/go/util/gs_test.go#n...
ct/go/util/gs_test.go:16: t.Skip("WARNING: Skipping this test due to
https://code.google.com/p/skia/issues/detail?id=3261")
On 2014/12/18 19:18:13, rmistry wrote:
> On 2014/12/18 19:12:42, borenet wrote:
> > Sorry to do this, but these failures are totally obscuring test output.
> 
> Joe, what is the right way to deal with this? we had decided on skipping it if
> it is a short test in:
>
https://codereview.chromium.org/779633003/diff/60001/ct/go/util/gs_test.go#ne...

IMO I should still be able to run the tests without -short and expect them to
pass instead of panicking.

[rmistry, 2014-12-18 19:54:38]

On 2014/12/18 19:19:23, borenet wrote:
> https://codereview.chromium.org/813443002/diff/180001/ct/go/util/gs_test.go
> File ct/go/util/gs_test.go (right):
> 
>
https://codereview.chromium.org/813443002/diff/180001/ct/go/util/gs_test.go#n...
> ct/go/util/gs_test.go:16: t.Skip("WARNING: Skipping this test due to
> https://code.google.com/p/skia/issues/detail?id=3261%22)
> On 2014/12/18 19:18:13, rmistry wrote:
> > On 2014/12/18 19:12:42, borenet wrote:
> > > Sorry to do this, but these failures are totally obscuring test output.
> > 
> > Joe, what is the right way to deal with this? we had decided on skipping it
if
> > it is a short test in:
> >
>
https://codereview.chromium.org/779633003/diff/60001/ct/go/util/gs_test.go#ne...
> 
> IMO I should still be able to run the tests without -short and expect them to
> pass instead of panicking.

Yes I'll do it as part of https://code.google.com/p/skia/issues/detail?id=3261
after this CL is in

[borenet, 2014-12-18 19:56:56]

On 2014/12/18 19:54:38, rmistry wrote:
> On 2014/12/18 19:19:23, borenet wrote:
> > https://codereview.chromium.org/813443002/diff/180001/ct/go/util/gs_test.go
> > File ct/go/util/gs_test.go (right):
> > 
> >
>
https://codereview.chromium.org/813443002/diff/180001/ct/go/util/gs_test.go#n...
> > ct/go/util/gs_test.go:16: t.Skip("WARNING: Skipping this test due to
> > https://code.google.com/p/skia/issues/detail?id=3261%22)
> > On 2014/12/18 19:18:13, rmistry wrote:
> > > On 2014/12/18 19:12:42, borenet wrote:
> > > > Sorry to do this, but these failures are totally obscuring test output.
> > > 
> > > Joe, what is the right way to deal with this? we had decided on skipping
it
> if
> > > it is a short test in:
> > >
> >
>
https://codereview.chromium.org/779633003/diff/60001/ct/go/util/gs_test.go#ne...
> > 
> > IMO I should still be able to run the tests without -short and expect them
to
> > pass instead of panicking.
> 
> Yes I'll do it as part of https://code.google.com/p/skia/issues/detail?id=3261
> after this CL is in

Unskipped in patch set 11.

[rmistry, 2014-12-18 19:58:19]

On 2014/12/18 19:56:56, borenet wrote:
> On 2014/12/18 19:54:38, rmistry wrote:
> > On 2014/12/18 19:19:23, borenet wrote:
> > >
https://codereview.chromium.org/813443002/diff/180001/ct/go/util/gs_test.go
> > > File ct/go/util/gs_test.go (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/813443002/diff/180001/ct/go/util/gs_test.go#n...
> > > ct/go/util/gs_test.go:16: t.Skip("WARNING: Skipping this test due to
> > > https://code.google.com/p/skia/issues/detail?id=3261%22)
> > > On 2014/12/18 19:18:13, rmistry wrote:
> > > > On 2014/12/18 19:12:42, borenet wrote:
> > > > > Sorry to do this, but these failures are totally obscuring test
output.
> > > > 
> > > > Joe, what is the right way to deal with this? we had decided on skipping
> it
> > if
> > > > it is a short test in:
> > > >
> > >
> >
>
https://codereview.chromium.org/779633003/diff/60001/ct/go/util/gs_test.go#ne...
> > > 
> > > IMO I should still be able to run the tests without -short and expect them
> to
> > > pass instead of panicking.
> > 
> > Yes I'll do it as part of
https://code.google.com/p/skia/issues/detail?id=3261
> > after this CL is in
> 
> Unskipped in patch set 11.

Sorry I meant leave it skipped so that things will work for you till I get to
the bug (not sure when that will be).

[borenet, 2014-12-18 20:01:08]

On 2014/12/18 19:58:19, rmistry wrote:
> On 2014/12/18 19:56:56, borenet wrote:
> > On 2014/12/18 19:54:38, rmistry wrote:
> > > On 2014/12/18 19:19:23, borenet wrote:
> > > >
> https://codereview.chromium.org/813443002/diff/180001/ct/go/util/gs_test.go
> > > > File ct/go/util/gs_test.go (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/813443002/diff/180001/ct/go/util/gs_test.go#n...
> > > > ct/go/util/gs_test.go:16: t.Skip("WARNING: Skipping this test due to
> > > > https://code.google.com/p/skia/issues/detail?id=3261%22)
> > > > On 2014/12/18 19:18:13, rmistry wrote:
> > > > > On 2014/12/18 19:12:42, borenet wrote:
> > > > > > Sorry to do this, but these failures are totally obscuring test
> output.
> > > > > 
> > > > > Joe, what is the right way to deal with this? we had decided on
skipping
> > it
> > > if
> > > > > it is a short test in:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/779633003/diff/60001/ct/go/util/gs_test.go#ne...
> > > > 
> > > > IMO I should still be able to run the tests without -short and expect
them
> > to
> > > > pass instead of panicking.
> > > 
> > > Yes I'll do it as part of
> https://code.google.com/p/skia/issues/detail?id=3261
> > > after this CL is in
> > 
> > Unskipped in patch set 11.
> 
> Sorry I meant leave it skipped so that things will work for you till I get to
> the bug (not sure when that will be).

I was following Joe's suggested pattern of guarding with -short and if you run
without -short and don't have things set up correctly, expect failure.  I'm okay
with the tests failing, it's just that the panic was making the output harder to
read.

[rmistry, 2014-12-18 20:04:46]

On 2014/12/18 20:01:08, borenet wrote:
> On 2014/12/18 19:58:19, rmistry wrote:
> > On 2014/12/18 19:56:56, borenet wrote:
> > > On 2014/12/18 19:54:38, rmistry wrote:
> > > > On 2014/12/18 19:19:23, borenet wrote:
> > > > >
> > https://codereview.chromium.org/813443002/diff/180001/ct/go/util/gs_test.go
> > > > > File ct/go/util/gs_test.go (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/813443002/diff/180001/ct/go/util/gs_test.go#n...
> > > > > ct/go/util/gs_test.go:16: t.Skip("WARNING: Skipping this test due to
> > > > > https://code.google.com/p/skia/issues/detail?id=3261%22)
> > > > > On 2014/12/18 19:18:13, rmistry wrote:
> > > > > > On 2014/12/18 19:12:42, borenet wrote:
> > > > > > > Sorry to do this, but these failures are totally obscuring test
> > output.
> > > > > > 
> > > > > > Joe, what is the right way to deal with this? we had decided on
> skipping
> > > it
> > > > if
> > > > > > it is a short test in:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/779633003/diff/60001/ct/go/util/gs_test.go#ne...
> > > > > 
> > > > > IMO I should still be able to run the tests without -short and expect
> them
> > > to
> > > > > pass instead of panicking.
> > > > 
> > > > Yes I'll do it as part of
> > https://code.google.com/p/skia/issues/detail?id=3261
> > > > after this CL is in
> > > 
> > > Unskipped in patch set 11.
> > 
> > Sorry I meant leave it skipped so that things will work for you till I get
to
> > the bug (not sure when that will be).
> 
> I was following Joe's suggested pattern of guarding with -short and if you run
> without -short and don't have things set up correctly, expect failure.  I'm
okay
> with the tests failing, it's just that the panic was making the output harder
to
> read.

Understood. Feel free to skip them if the output gets annoying.

[borenet, 2014-12-18 22:10:46]

Enhancements to migratedb. PTAL

[borenet, 2014-12-19 17:17:34]

Added host/port/user/database name flags to database package.  Prompt for
password or get it from metadata based on --local flag.

[jcgregorio, 2014-12-19 20:27:06]

https://codereview.chromium.org/813443002/diff/200001/README.md
File README.md (right):

https://codereview.chromium.org/813443002/diff/200001/README.md#newcode12
README.md:12: $ ./setup_test_db
Requires a local installation of MySQL. For a Debian based distro:

  $ sudo apt-get install mysql-client mysql-server

https://codereview.chromium.org/813443002/diff/320001/README.md
File README.md (right):

https://codereview.chromium.org/813443002/diff/320001/README.md#newcode20
README.md:20: setup_test_db script is included in go/database for convenience in
setting up
The script `setup_test_db` in `go/database` is

https://codereview.chromium.org/813443002/diff/320001/go/database/database.go
File go/database/database.go (right):

https://codereview.chromium.org/813443002/diff/320001/go/database/database.go...
go/database/database.go:10: 
no blank line here

Not sure why goimports always puts "strings" on it's own.

https://codereview.chromium.org/813443002/diff/320001/go/database/database.go...
go/database/database.go:63: // if local is true, the DB host is overridden.
If

https://codereview.chromium.org/813443002/diff/320001/go/database/testutil/te...
File go/database/testutil/testutil.go (right):

https://codereview.chromium.org/813443002/diff/320001/go/database/testutil/te...
go/database/testutil/testutil.go:44: // LocalTestDatabaseConfig returns a
DatabaseConfig appropriate for local
 LocalTestRootDatabaseConfig

https://codereview.chromium.org/813443002/diff/320001/golden/go/expstorage/ex...
File golden/go/expstorage/expstorage_test.go (right):

https://codereview.chromium.org/813443002/diff/320001/golden/go/expstorage/ex...
golden/go/expstorage/expstorage_test.go:23: vdb := database.NewVersionedDB(conf)
Why not make MySQLTestDatabase rootVdb public and then replace vdb with
testDb.RootVdb?

https://codereview.chromium.org/813443002/diff/320001/perf/DESIGN.md
File perf/DESIGN.md (right):

https://codereview.chromium.org/813443002/diff/320001/perf/DESIGN.md#newcode222
perf/DESIGN.md:222: $ migratedb -logtostderr=true -db_host=localhost
$ perf_migratedb -logtostderr=true -db_host=localhost

[borenet, 2014-12-19 20:39:15]

Uploaded patch set 18

https://codereview.chromium.org/813443002/diff/200001/README.md
File README.md (right):

https://codereview.chromium.org/813443002/diff/200001/README.md#newcode12
README.md:12: $ ./setup_test_db
On 2014/12/19 20:27:05, jcgregorio wrote:
> Requires a local installation of MySQL. For a Debian based distro:
> 
>   $ sudo apt-get install mysql-client mysql-server
> 

Done.

https://codereview.chromium.org/813443002/diff/320001/README.md
File README.md (right):

https://codereview.chromium.org/813443002/diff/320001/README.md#newcode20
README.md:20: setup_test_db script is included in go/database for convenience in
setting up
On 2014/12/19 20:27:05, jcgregorio wrote:
> The script `setup_test_db` in `go/database` is

Done.

https://codereview.chromium.org/813443002/diff/320001/go/database/database.go
File go/database/database.go (right):

https://codereview.chromium.org/813443002/diff/320001/go/database/database.go...
go/database/database.go:10: 
On 2014/12/19 20:27:06, jcgregorio wrote:
> no blank line here
> 
> Not sure why goimports always puts "strings" on it's own.

Done.

https://codereview.chromium.org/813443002/diff/320001/go/database/database.go...
go/database/database.go:63: // if local is true, the DB host is overridden.
On 2014/12/19 20:27:05, jcgregorio wrote:
> If

Done.

https://codereview.chromium.org/813443002/diff/320001/go/database/testutil/te...
File go/database/testutil/testutil.go (right):

https://codereview.chromium.org/813443002/diff/320001/go/database/testutil/te...
go/database/testutil/testutil.go:44: // LocalTestDatabaseConfig returns a
DatabaseConfig appropriate for local
On 2014/12/19 20:27:06, jcgregorio wrote:
>  LocalTestRootDatabaseConfig

Done.

https://codereview.chromium.org/813443002/diff/320001/golden/go/expstorage/ex...
File golden/go/expstorage/expstorage_test.go (right):

https://codereview.chromium.org/813443002/diff/320001/golden/go/expstorage/ex...
golden/go/expstorage/expstorage_test.go:23: vdb := database.NewVersionedDB(conf)
On 2014/12/19 20:27:06, jcgregorio wrote:
> Why not make MySQLTestDatabase rootVdb public and then replace vdb with
> testDb.RootVdb?

That would work, but the test would be running as the root user.  May not matter
but does matter for others (like the database versioning tests).  I actually
prefer forcing the test to decide for itself.

https://codereview.chromium.org/813443002/diff/320001/perf/DESIGN.md
File perf/DESIGN.md (right):

https://codereview.chromium.org/813443002/diff/320001/perf/DESIGN.md#newcode222
perf/DESIGN.md:222: $ migratedb -logtostderr=true -db_host=localhost
On 2014/12/19 20:27:06, jcgregorio wrote:
> $ perf_migratedb -logtostderr=true -db_host=localhost

Done.

[borenet, 2015-01-05 16:32:03]

Friendly post-vacation ping.

[jcgregorio, 2015-01-05 18:33:18]

https://codereview.chromium.org/813443002/diff/360001/go/database/database.go
File go/database/database.go (right):

https://codereview.chromium.org/813443002/diff/360001/go/database/database.go...
go/database/database.go:77: fmt.Printf("Enter password for MySQL user %s at
%s:%d: ", *dbUser, useHost, *dbPort)
Can we skip the prompt if we are local and presume the password is always ""?

[borenet, 2015-01-05 19:03:20]

https://codereview.chromium.org/813443002/diff/360001/go/database/database.go
File go/database/database.go (right):

https://codereview.chromium.org/813443002/diff/360001/go/database/database.go...
go/database/database.go:77: fmt.Printf("Enter password for MySQL user %s at
%s:%d: ", *dbUser, useHost, *dbPort)
On 2015/01/05 18:33:18, jcgregorio wrote:
> Can we skip the prompt if we are local and presume the password is always ""?

That's fine with me - the caller can always prompt beforehand and set the
password.

[borenet, 2015-01-05 19:04:17]

On 2015/01/05 19:03:20, borenet wrote:
> https://codereview.chromium.org/813443002/diff/360001/go/database/database.go
> File go/database/database.go (right):
> 
>
https://codereview.chromium.org/813443002/diff/360001/go/database/database.go...
> go/database/database.go:77: fmt.Printf("Enter password for MySQL user %s at
> %s:%d: ", *dbUser, useHost, *dbPort)
> On 2015/01/05 18:33:18, jcgregorio wrote:
> > Can we skip the prompt if we are local and presume the password is always
""?
> 
> That's fine with me - the caller can always prompt beforehand and set the
> password.

(uploaded patch set 20)

[jcgregorio, 2015-01-05 20:48:06]

On 2015/01/05 19:04:17, borenet wrote:
> On 2015/01/05 19:03:20, borenet wrote:
> >
https://codereview.chromium.org/813443002/diff/360001/go/database/database.go
> > File go/database/database.go (right):
> > 
> >
>
https://codereview.chromium.org/813443002/diff/360001/go/database/database.go...
> > go/database/database.go:77: fmt.Printf("Enter password for MySQL user %s at
> > %s:%d: ", *dbUser, useHost, *dbPort)
> > On 2015/01/05 18:33:18, jcgregorio wrote:
> > > Can we skip the prompt if we are local and presume the password is always
> ""?
> > 
> > That's fine with me - the caller can always prompt beforehand and set the
> > password.
> 
> (uploaded patch set 20)

LGTM

[borenet, 2015-01-05 20:58:51]

The CQ bit was checked by borenet@google.com

[commit-bot: I haz the power, 2015-01-05 20:59:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/813443002/380001

[commit-bot: I haz the power, 2015-01-05 20:59:16]

Committed patchset #20 (id:380001) as
https://skia.googlesource.com/buildbot/+/bee4e6a84182c691f7ea2c984d54d2ece023...