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 support for finagle mysql #6

Merged
merged 24 commits into from
Feb 23, 2015
Merged

Add support for finagle mysql #6

merged 24 commits into from
Feb 23, 2015

Conversation

bguthrie
Copy link
Contributor

@bguthrie bguthrie commented Jan 7, 2015

Submitted for a first look; feedback welcome.

@samn
Copy link
Collaborator

samn commented Jan 9, 2015

this looks pretty awesome! i'll take a closer look this weekend

@bguthrie
Copy link
Contributor Author

bguthrie commented Jan 9, 2015

I have to figure out how to get it working with Travis MySQL; I've reviewed their docs and believe I'm using the right creds but I'll keep poking.

(with-credentials "test" "test")
(with-database "some_database")
(rich-client "localhost:3306"))]
(-> (prepare "SELECT * FROM widgets WHERE id = ? LIMIT 1")
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't prepare have db as its first argument?

@samn
Copy link
Collaborator

samn commented Jan 10, 2015

This is really awesome Brian! a couple of small things to fixup besides getting this to run against mysql in CI. Not sure why it isn't working either, looks like you are doing the correct thing according to their docs.

@bguthrie
Copy link
Contributor Author

I think this is a limitation more generally of finagle-mysql, which seems
to insist on non-empty passwords. But since the upstream finagle also uses
Travis I'm not sure what the problem is. I didn't see any MySQL in use with
their Travis build.

Thanks for the feedback! Will push those fixes soon.

On Saturday, January 10, 2015, Sam Neubardt notifications@github.com
wrote:

This is really awesome Brian! a couple of small things to fixup besides
getting this to run against mysql in CI. Not sure why it isn't working
either, looks like you are doing the correct thing according to their docs.


Reply to this email directly or view it on GitHub
#6 (comment).

@samn
Copy link
Collaborator

samn commented Jan 11, 2015

could you add another root user with a password in the travis before_script?

@bguthrie
Copy link
Contributor Author

That's a great idea, I'll give it a shot.

On Sunday, January 11, 2015, Sam Neubardt notifications@github.com wrote:

could you add another root user with a password in the travis
before_script?


Reply to this email directly or view it on GitHub
#6 (comment).

@samn
Copy link
Collaborator

samn commented Feb 2, 2015

Had any luck Brian? Want any help getting CI set up?

@samn
Copy link
Collaborator

samn commented Feb 2, 2015

also i'd be cool with removing the integration test and assuming the underlying framework works ok.

@bguthrie
Copy link
Contributor Author

bguthrie commented Feb 3, 2015

I'd prefer to keep the tests in, but I'm just not sure how to diagnose this issue. I've tried reconfiguring the tests a few different ways, and apparently the user and database creation bits are working just fine. I'm not at all sure why they'd be okay but the table is nonetheless missing, and apparently there's no real way to replicate a Travis build locally. I'll keep trying.

@bguthrie
Copy link
Contributor Author

@samn VICTORY. The issue was case-sensitivity; it took a while to figure out because the stacktrace was entirely unhelpful.

@samn
Copy link
Collaborator

samn commented Feb 20, 2015

Awesome!! Thanks for not giving up on this. I'll give this a once over this weekend and likely do a new release with this.

@bguthrie
Copy link
Contributor Author

Nice! 🚢

a Clojure vector of maps"
(->> (.rows rs)
(scala/scala-seq->vec)
(map Row->map)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

won't this return a (lazy) seq, not a vector?

Copy link
Collaborator

Choose a reason for hiding this comment

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

changed this to use mapv and return a vector in f6a8971

@samn
Copy link
Collaborator

samn commented Feb 23, 2015

This is so awesome. Thanks for your hard work on this @bguthrie!

samn added a commit that referenced this pull request Feb 23, 2015
@samn samn merged commit c635fc2 into finagle:master Feb 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants