[GitHub] chetanmeh commented on a change in pull request #3369: Use pureconfig for CouchDbRestStore
chetanmeh commented on a change in pull request #3369: Use pureconfig for
CouchDbRestStore
URL:
https://github.com/apache/incubator-openwhisk/pull/3369#discussion_r172020848
##
File path:
tests/src/test/scala/whisk/core/controller/test/ActivationsApiTests.scala
##
@@ -526,18 +526,33 @@ class ActivationsApiTests extends ControllerTestCommon
with WhiskActivationsApi
implicit val materializer = ActorMaterializer()
val activationStore = SpiLoader
.get[ArtifactStoreProvider]
- .makeStore[WhiskEntity](whiskConfig, _.dbActivations)(
-classTag[WhiskEntity],
-WhiskEntityJsonFormat,
+ .makeStore[WhiskActivation]()(
+classTag[WhiskActivation],
+WhiskActivation.serdes,
WhiskDocumentReader,
system,
logging,
materializer)
implicit val tid = transid()
-val entity = BadEntity(namespace, EntityName(ActivationId().toString))
-put(activationStore, entity)
-Get(s"$collectionPath/${entity.name}") ~> Route.seal(routes(creds)) ~>
check {
+class BadActivation(override val namespace: EntityPath,
Review comment:
The test here was validating that deserialization issue triggers 500
response. For this it injected a WhiskEntity in Activation table by "misusing"
the dbName parameter
```
val activationStore = SpiLoader
.get[ArtifactStoreProvider]
.makeStore[WhiskEntity](whiskConfig, _.dbActivations)(
WhiskEntityJsonFormat,
```
Now as the db name is now mapped implicitly by entity type class this would
not work. So test has been modified to use a `BadActivation` which emits wrong
json and thus triggers the deserialization fault
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] chetanmeh commented on a change in pull request #3369: Use pureconfig for CouchDbRestStore
chetanmeh commented on a change in pull request #3369: Use pureconfig for
CouchDbRestStore
URL:
https://github.com/apache/incubator-openwhisk/pull/3369#discussion_r172020722
##
File path:
common/scala/src/main/scala/whisk/core/database/CouchDbStoreProvider.scala
##
@@ -21,37 +21,49 @@ import akka.actor.ActorSystem
import akka.stream.ActorMaterializer
import spray.json.RootJsonFormat
import whisk.common.Logging
-import whisk.core.WhiskConfig
+import whisk.core.ConfigKeys
import whisk.core.entity.DocumentReader
+import pureconfig._
import scala.reflect.ClassTag
+case class CouchDbConfig(provider: String,
+ protocol: String,
+ host: String,
+ port: Int,
+ username: String,
+ password: String,
+ databases: Map[String, String]) {
+ assume(Set(protocol, host, username, password).forall(_.nonEmpty), "At least
one expected property is missing")
+
+ def getDatabaseName(entityClass: Class[_]): String = {
Review comment:
Good point. Updated PR with above patch
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] chetanmeh commented on a change in pull request #3369: Use pureconfig for CouchDbRestStore
chetanmeh commented on a change in pull request #3369: Use pureconfig for
CouchDbRestStore
URL:
https://github.com/apache/incubator-openwhisk/pull/3369#discussion_r172005653
##
File path: common/scala/src/main/resources/application.conf
##
@@ -106,6 +106,23 @@ whisk {
activations-ddoc = "whisks.v2.1.0"
activations-filter-ddoc = "whisks-filters.v2.1.0"
}
+
+# CouchDB related configuration
Review comment:
The design docs are used by layers above `ArtifactStore` and hence not
internal to it. Infact the view names are used for other [custom stores][1] to
determine how to frame the queries.
So they need to be kept separately
[1]:
https://github.com/chetanmeh/openwhisk-mongo/blob/master/common/mongo/src/main/scala/whisk/core/database/mongo/MongoViewMapper.scala
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] chetanmeh commented on a change in pull request #3369: Use pureconfig for CouchDbRestStore
chetanmeh commented on a change in pull request #3369: Use pureconfig for
CouchDbRestStore
URL:
https://github.com/apache/incubator-openwhisk/pull/3369#discussion_r172005583
##
File path: tests/src/test/scala/ha/ShootComponentsTests.scala
##
@@ -67,13 +70,13 @@ class ShootComponentsTests
val controller0DockerHost = WhiskProperties.getBaseControllerHost()
val couchDB0DockerHost = WhiskProperties.getBaseDBHost()
- val dbProtocol = WhiskProperties.getProperty(WhiskConfig.dbProtocol)
+ val dbConfig = loadConfigOrThrow[CouchDbConfig](ConfigKeys.couchdb)
+ val dbProtocol = dbConfig.protocol
val dbHostsList = WhiskProperties.getDBHosts
- val dbPort = WhiskProperties.getProperty(WhiskConfig.dbPort)
- val dbUsername = WhiskProperties.getProperty(WhiskConfig.dbUsername)
- val dbPassword = WhiskProperties.getProperty(WhiskConfig.dbPassword)
- val dbPrefix = WhiskProperties.getProperty(WhiskConfig.dbPrefix)
- val dbWhiskAuth = WhiskProperties.getProperty(WhiskConfig.dbAuths)
+ val dbPort = dbConfig.port
+ val dbUsername = dbConfig.username
+ val dbPassword = dbConfig.password
+ val dbWhiskAuth = dbConfig.databases.get("WhiskAuth").get
Review comment:
Problem here is `WhiskAuth` is scoped to `core` package and test lives
outside of that package. So need to use the string value explicitly
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] chetanmeh commented on a change in pull request #3369: Use pureconfig for CouchDbRestStore
chetanmeh commented on a change in pull request #3369: Use pureconfig for
CouchDbRestStore
URL:
https://github.com/apache/incubator-openwhisk/pull/3369#discussion_r171225153
##
File path:
common/scala/src/main/scala/whisk/core/database/CouchDbStoreProvider.scala
##
@@ -36,21 +47,18 @@ object CouchDbStoreProvider extends ArtifactStoreProvider {
actorSystem: ActorSystem,
logging: Logging,
materializer: ActorMaterializer): ArtifactStore[D] = {
-require(config != null && config.isValid, "config is undefined or not
valid")
+val dbConfig = loadConfigOrThrow[CouchDbConfig](ConfigKeys.couchdb)
require(
- config.dbProvider == "Cloudant" || config.dbProvider == "CouchDB",
- "Unsupported db.provider: " + config.dbProvider)
-assume(
- Set(config.dbProtocol, config.dbHost, config.dbPort, config.dbUsername,
config.dbPassword, name(config))
-.forall(_.nonEmpty),
- "At least one expected property is missing")
+ dbConfig.provider == "Cloudant" || dbConfig.provider == "CouchDB",
+ s"Unsupported db.provider: ${dbConfig.provider}")
Review comment:
May be we deal with this separately as I see this distinction between
CouchDB and Cloudant in few places like
1. CloudantRestClient
2. Vagrantfile
3. CouchDbRestClientTests
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] chetanmeh commented on a change in pull request #3369: Use pureconfig for CouchDbRestStore
chetanmeh commented on a change in pull request #3369: Use pureconfig for
CouchDbRestStore
URL:
https://github.com/apache/incubator-openwhisk/pull/3369#discussion_r171209657
##
File path:
common/scala/src/main/scala/whisk/core/database/CouchDbStoreProvider.scala
##
@@ -36,21 +47,18 @@ object CouchDbStoreProvider extends ArtifactStoreProvider {
actorSystem: ActorSystem,
logging: Logging,
materializer: ActorMaterializer): ArtifactStore[D] = {
-require(config != null && config.isValid, "config is undefined or not
valid")
+val dbConfig = loadConfigOrThrow[CouchDbConfig](ConfigKeys.couchdb)
require(
- config.dbProvider == "Cloudant" || config.dbProvider == "CouchDB",
- "Unsupported db.provider: " + config.dbProvider)
-assume(
- Set(config.dbProtocol, config.dbHost, config.dbPort, config.dbUsername,
config.dbPassword, name(config))
-.forall(_.nonEmpty),
- "At least one expected property is missing")
+ dbConfig.provider == "Cloudant" || dbConfig.provider == "CouchDB",
+ s"Unsupported db.provider: ${dbConfig.provider}")
Review comment:
Not sure what is the intention of provider so far. In code this param is not
used. I just kept existing logic as is with switch to pureconfig. If provider
is not required (if others can confirm) I can then remove that also
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
