Adds Finatra for skeleton api #45
Conversation
| } | ||
| } | ||
|
|
||
| object ElasticClientModule extends TwitterModule { |
There was a problem hiding this comment.
@jamesgorrie take a look at how Finatra does dependency injection ... it's beautiful.
app/build.sbt
Outdated
| "com.typesafe" % "config" % "1.3.1", | ||
| "org.elasticsearch" % "elasticsearch" % "5.1.2", | ||
| "com.sksamuel.elastic4s" %% "elastic4s-core" % "5.1.5", | ||
| "com.sksamuel.elastic4s" %% "elastic4s-xpack-security" % "5.1.5", |
There was a problem hiding this comment.
Any reason we don’t incorporate these versions into the versions field above?
| calmService: CalmService | ||
| ) extends Controller { | ||
|
|
||
| val apiBaseUrl = "/api/v0" |
There was a problem hiding this comment.
Not that it’s hugely important right now, but I think the consensus was we drop the /api from URLs, as it’s all being served from api.wc.org.
| val recordCollectionPair = for { | ||
| recordOption <- calmService.findRecordByAltRefNo(request.altRefNo) | ||
| collectionOption <- calmService.findParentCollectionByAltRefNo(request.altRefNo) | ||
| } yield RecordCollectionPair(recordOption, collectionOption) |
There was a problem hiding this comment.
Am I right in thinking that recordOption and collectionOption are futures here, so we don’t hit the yield until both have resolved? Or does RecordCollectionPair become its own future?
| title=data("Title").toString, | ||
| materialType=data("Material").toString, | ||
| date=data.get("Date").map(_.toString), | ||
| acquisition=data.get("Acquisition").map(_.toString) |
There was a problem hiding this comment.
What’s the difference between foo.toString and foo.map(_.toString)? Is it something to do with futures (and if so, why are only some of the fields futures?). Or because some of the fields are absent/null?
| server.httpGet( | ||
| path = "/", | ||
| andExpect = Ok, | ||
| withBody = "{\"message\":\"success\"}") |
| override def toResponse(request: Request, exception: ElasticsearchException): Response = { | ||
| val errorResponse = ErrorResponse(exception.getMessage) | ||
|
|
||
| response.internalServerError.json(errorResponse) |
There was a problem hiding this comment.
This exposes error messages from the Elasticsearch library to end-users. It would be better if this logged the error message, and then we just returned a generic 500 error.
| elasticsearchService.client | ||
| .execute { clusterHealth() } | ||
| .map { response.ok.json } | ||
| } |
There was a problem hiding this comment.
Open Q: do we want to expose these management endpoints? To end-users: definitely not. To internal staff: we should talk to James et al. about what they’re doing with healthcheck/monitoring and so on.
| collection: Option[Collection] | ||
| ) | ||
|
|
||
| case class RecordResponse( |
There was a problem hiding this comment.
Consider defining these elsewhere?
|
|
||
| @Singleton | ||
| class MainController @Inject()( | ||
| calmService: CalmService |
There was a problem hiding this comment.
Open Q: how does an instance of CalmService actually appear here?
Adds a BuildEnv plugin and an configS3JavaOpts plugin. The latter retrives configuration in the typesage config library format from S3 and returns a series of Java option flags suitable for passing to sbt-native-packager. The result is that when building a docker image for this application it will bake configuration from S3 in the Typesafe config format into the image for the appropriate environment in a format that Finatra processes as flags.
- Fix test package namespaces - Add resolvers for elasticsearch xpack - Use common version for elastic4s packages
kenoir
changed the title
Slightly tidy some of the Terraform
Addresses #16