Allow Redshift user and password to be passed as options #162

tristanreid · 2016-01-24T01:37:18Z

Per #132, this PR adds user and password options. If these options are present and the user/password are still in the URL, an exception is thrown with a clear error message.

Unit tests are still pending - maybe it's better to check for ambiguous parameters in the Parameter class instead, to make it easier to test? Glad for any other feedback as well!

JoshRosen · 2016-01-24T02:19:28Z

@tristanreid, great idea regarding doing the validation in the Parameter itself; it looks like we already do this to some degree, so it should be a small change to do it there: https://github.com/tristanreid/spark-redshift/blob/5210e7ea1169c1351fc6897082cdd3d347a81a2b/src/main/scala/com/databricks/spark/redshift/Parameters.scala#L55

JoshRosen · 2016-01-24T02:20:14Z

README.md

Super minor style nit, but I'd probably change the comma to a semicolon.

JoshRosen · 2016-01-24T02:23:22Z

By the way, this looks good overall; my review comments above are mostly minor nits and should be easy to address; no fundamental code changes needed.

tristanreid · 2016-01-25T14:31:34Z

Thanks for the great feedback, Josh. I can get that unit test worked out in the next day or so if you don't get to it.

JoshRosen · 2016-01-25T19:00:13Z

I'd add the test in ParametersSuite, modeled after this one: https://github.com/tristanreid/spark-redshift/blob/master/src/test/scala/com/databricks/spark/redshift/ParametersSuite.scala#L68

JoshRosen · 2016-01-25T23:57:47Z

src/main/scala/com/databricks/spark/redshift/Parameters.scala

I'd use if (credsInURL.isDefined) here.

JoshRosen · 2016-01-26T00:00:11Z

Whoops, missed your commit comment earlier. Aside from the minor style nits above, this change looks good to me.

tristanreid · 2016-01-26T18:23:39Z

Thanks for the great feedback on this! Should be good to go. By the way, is there a lint / stylechecker available for the Databricks style guide? If not, the rules look pretty easy for me to add on my own, I'm just being lazy (as a good programmer should, of course).

JoshRosen · 2016-01-26T19:08:47Z

(Closed and re-opened to trigger Travis)

JoshRosen · 2016-01-26T19:14:52Z

src/main/scala/com/databricks/spark/redshift/Parameters.scala

Scalastyle is complaining about this:

[error] /home/travis/build/databricks/spark-redshift/src/main/scala/com/databricks/spark/redshift/Parameters.scala:66:11: No space after token if

JoshRosen · 2016-01-26T19:15:09Z

Looks like I had Travis disabled for PRs, but should be fixed now.

codecov-io · 2016-01-26T20:04:28Z

Current coverage is `87.53%`

Merging #162 into master will decrease coverage by -1.44% as of fcf1526

@@            master    #162   diff @@
======================================
  Files           13      13       
  Stmts          644     658    +14
  Branches       142     146     +4
  Methods          0       0       
======================================
+ Hit            573     576     +3
  Partial          0       0       
- Missed          71      82    +11

Review entire Coverage Diff as of fcf1526

Powered by Codecov. Updated on successful CI builds.

JoshRosen · 2016-01-26T20:07:20Z

(Triggering end-to-end integration tests)

JoshRosen · 2016-01-26T20:16:11Z

Weird:

[info] Exception encountered when attempting to run a suite with class name: com.databricks.spark.redshift.DecimalIntegrationSuite *** ABORTED ***
[info]   scala.MatchError: None (of class scala.None$)
[info]   at com.databricks.spark.redshift.JDBCWrapper.getConnector(RedshiftJDBCWrapper.scala:229)
[info]   at com.databricks.spark.redshift.IntegrationSuiteBase$class.beforeAll(IntegrationSuiteBase.scala:91)
[info]   at com.databricks.spark.redshift.DecimalIntegrationSuite.beforeAll(DecimalIntegrationSuite.scala:25)
[info]   at org.scalatest.BeforeAndAfterAll$class.beforeAll(BeforeAndAfterAll.scala:187)
[info]   at com.databricks.spark.redshift.DecimalIntegrationSuite.beforeAll(DecimalIntegrationSuite.scala:25)
[info]   at org.scalatest.BeforeAndAfterAll$class.run(BeforeAndAfterAll.scala:253)
[info]   at com.databricks.spark.redshift.DecimalIntegrationSuite.run(DecimalIntegrationSuite.scala:25)
[info]   at org.scalatest.tools.Framework.org$scalatest$tools$Framework$$runSuite(Framework.scala:462)
[info]   at org.scalatest.tools.Framework$ScalaTestTask.execute(Framework.scala:671)
[info]   at sbt.ForkMain$Run$2.call(ForkMain.java:296)
[info]   at sbt.ForkMain$Run$2.call(ForkMain.java:286)
[info]   at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:334)
[info]   at java.util.concurrent.FutureTask.run(FutureTask.java:166)
[info]   at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1146)
[info]   at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
[info]   at java.lang.Thread.run(Thread.java:701)

tristanreid · 2016-01-26T23:27:27Z

Ugh...sorry it's taking so many iterations. I don't have the environment to run the integration tests at the moment (in particular the AWS session token)

tristanreid · 2016-01-27T17:09:33Z

Is this good to go?

JoshRosen · 2016-01-27T17:15:42Z

src/main/scala/com/databricks/spark/redshift/RedshiftJDBCWrapper.scala

Should use credentials.foreach { case (user, password => } instead.

JoshRosen · 2016-01-27T17:18:02Z

I'll try to take another look later today

tristanreid · 2016-01-29T18:38:52Z

Hey Josh,
Any thoughts or changes? Thanks

JoshRosen · 2016-01-30T00:16:39Z

src/main/scala/com/databricks/spark/redshift/RedshiftJDBCWrapper.scala

According to codecov, it looks like this was never actually hit during tests. I'm going to go ahead and merge this now and will add an end-to-end test myself in a followup patch.

JoshRosen · 2016-01-30T00:17:06Z

This looks fine to me. There's one extra test that I would add, but I can do it myself in a followup. Thanks!

…ather than URI Followup to #162 to add a roundtrip test case in order to improve code coverage. Author: Josh Rosen <joshrosen@databricks.com> Closes #166 from JoshRosen/more-credential-tests.

Tristan Reid added 2 commits January 23, 2016 11:09

add credentials parameter to JDBCWrapper.getConnector

8fe5c13

document user/password options in README

5210e7e

JoshRosen reviewed Jan 24, 2016
View reviewed changes

README.md

Copy link

Contributor

JoshRosen Jan 24, 2016

Choose a reason for hiding this comment

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

Super minor style nit, but I'd probably change the comma to a semicolon.

Tristan Reid added 3 commits January 25, 2016 06:24

without punctuation, we're just cavemen

fa25a0a

check for superfluous credentials in Parameters.scala

41fdcf3

wrap parameters and remove check for superflous credentials

4873533

Tristan Reid added 2 commits January 25, 2016 13:08

throw exception if Redshift credentials are not passed

a22e3b5

unit tests for Redshift credentials

0d3472d

JoshRosen reviewed Jan 25, 2016
View reviewed changes

src/main/scala/com/databricks/spark/redshift/Parameters.scala Outdated

Copy link

Contributor

JoshRosen Jan 25, 2016

Choose a reason for hiding this comment

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

I'd use if (credsInURL.isDefined) here.

Fixed stylistic issues, and use isDefined on Option rather than foreach

1fc5457

JoshRosen added this to the 0.6.1 milestone Jan 26, 2016

JoshRosen added the enhancement label Jan 26, 2016

JoshRosen self-assigned this Jan 26, 2016

JoshRosen closed this Jan 26, 2016

JoshRosen reopened this Jan 26, 2016

JoshRosen reviewed Jan 26, 2016
View reviewed changes

Tristan Reid added 2 commits January 26, 2016 11:25

fix style error: space after if

9a488c2

fix style issues - this time I checked w/ sbt scalastyle

242b236

catch None

1da95b9

JoshRosen reviewed Jan 27, 2016
View reviewed changes

replace match with foreach

fcf1526

JoshRosen reviewed Jan 30, 2016
View reviewed changes

JoshRosen closed this in 65999ea Jan 30, 2016

This was referenced Jan 30, 2016

Redshift password in connection string can't be illegal-URL characters #132

Closed

Add roundtrip test for specifying Redshift JDBC credentials in conf rather than URI #166

Closed

Allow Redshift user and password to be passed as options #162

Allow Redshift user and password to be passed as options #162

Uh oh!

Conversation

tristanreid commented Jan 24, 2016

Uh oh!

JoshRosen commented Jan 24, 2016

Uh oh!

JoshRosen Jan 24, 2016

Choose a reason for hiding this comment

Uh oh!

JoshRosen commented Jan 24, 2016

Uh oh!

tristanreid commented Jan 25, 2016

Uh oh!

JoshRosen commented Jan 25, 2016

Uh oh!

JoshRosen Jan 25, 2016

Choose a reason for hiding this comment

Uh oh!

JoshRosen commented Jan 26, 2016

Uh oh!

tristanreid commented Jan 26, 2016

Uh oh!

JoshRosen commented Jan 26, 2016

Uh oh!

JoshRosen Jan 26, 2016

Choose a reason for hiding this comment

Uh oh!

JoshRosen commented Jan 26, 2016

Uh oh!

codecov-io commented Jan 26, 2016

Current coverage is 87.53%

Uh oh!

JoshRosen commented Jan 26, 2016

Uh oh!

JoshRosen commented Jan 26, 2016

Uh oh!

tristanreid commented Jan 26, 2016

Uh oh!

tristanreid commented Jan 27, 2016

Uh oh!

JoshRosen Jan 27, 2016

Choose a reason for hiding this comment

Uh oh!

tristanreid Jan 28, 2016

Choose a reason for hiding this comment

Uh oh!

JoshRosen commented Jan 27, 2016

Uh oh!

tristanreid commented Jan 29, 2016

Uh oh!

JoshRosen Jan 30, 2016

Choose a reason for hiding this comment

Uh oh!

JoshRosen commented Jan 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Current coverage is `87.53%`