Add support for execution of multiple statements in one migration file #276
Conversation
syndbg
force-pushed
the
cassandra-enhancement
branch
from
6445880 to
455d6f5
Compare
|
Note that I'll take a look at the failing tests and add/modify as needed. |
syndbg
force-pushed
the
cassandra-enhancement
branch
from
455d6f5 to
d17e048
Compare
database/cassandra/cassandra.go
Outdated
| username = u.User.Username() | ||
| password, _ = u.User.Password() | ||
| } else { | ||
| username = "cassandra" |
There was a problem hiding this comment.
where is this default coming from?
There was a problem hiding this comment.
It's the default username/password when enabling PasswordAuthentication authenticator in Cassandra. See https://docs.datastax.com/en/cassandra/2.1/cassandra/security/security_config_native_authenticate_t.html.
|
Thanks for your PR. I think the tests are failing somewhere else actually. Need to investigate as well. |
| if err := p.session.Query(query).Exec(); err != nil { | ||
| // TODO: cast to Cassandra error and get line number | ||
| return database.Error{OrigErr: err, Err: "migration failed", Query: migr} | ||
| matches := regexp.MustCompile(`(?m:;$)`).Split(query, -1) |
There was a problem hiding this comment.
It didn't execute multiple queries before?
There was a problem hiding this comment.
I just submitted #280 (but didn't know about this PR until now) which has an opt-in when it comes to multi-statement support to avoid the case of
INSERT INTO codesamples(code) VALUES ('import a;
import b;');Might be worth considering.
|
@JensRantil thanks for your PRs. I closed #280 in favor of this one. I'd much rather find a regex than introducing custom query markup. (Partially related: FAQ:why-two-separate-files) @syndbg and @JensRantil thanks for bringing this up. |
|
Any progress on how to handle strings with newlines? We are considering forking |
|
@JensRantil I totally slept on this PR. Give me a day and I'll finally address everything left. |
syndbg
force-pushed
the
cassandra-enhancement
branch
from
f328b74 to
02b495b
Compare
syndbg
changed the title
syndbg
force-pushed
the
cassandra-enhancement
branch
from
02b495b to
2195521
Compare
Wasn't able to run them locally due to `gocql` not able to connect at all. Instead, force Cassandra container to broadcast to localhost and auth with default credentials.
|
@JensRantil @mattes Updated and cleaned up. Cassandra tests still fail and I can't figure out. (note that they were failing before the changes too) I had troubles getting them running locally, but I got this working eventually. Still I can't find why they're considered a |
| if err := p.session.Query(query).Exec(); err != nil { | ||
| // TODO: cast to Cassandra error and get line number | ||
| return database.Error{OrigErr: err, Err: "migration failed", Query: migr} | ||
| matches := regexp.MustCompile(`(?m:;$)`).Split(query, -1) |
There was a problem hiding this comment.
How about adding a few unit tests to make sure that this works as expected?
There was a problem hiding this comment.
I had troubles getting them running locally, but I got this working eventually. Still I can't find why they're considered a FAIL.
Atm the tests fail for me even before the changes.
I'll spend more time to see why they're failing before I start to add more tests (that are probably going to fail either way).
|
Any updates on merging this in? |
| "time" | ||
| nurl "net/url" |
|
@afking It's quite stale. It was ready to merge in November 2017. Idk about now, it's probably obsolete/out-dated. |
|
@syndbg thanks, i've applied the changes locally. |
Authentication functionality,(added in feat(cassandra): support for user/pw authentication #278)@mattes Just something that prevented us from using
migrateinitially.