Call caching endpoint #306
Conversation
| * Assertions on call FQN and index: | ||
| * <ul> | ||
| * <li>Call with this name must exist in the workflow</li> | ||
| * <li>Call must not correspond to a collector</li> |
There was a problem hiding this comment.
Wouldn't this be fine? If you try to disable a call that was a scatter, disable all shards?
There was a problem hiding this comment.
open for discussion. 😄
|
|
||
| private [webservice] def validateCallName(callName: Option[String]): ValidationNel[String, Option[ExecutionDatabaseKey]] = { | ||
| callName map { name => | ||
| val CallNamePattern = "((?:[a-zA-Z][a-zA-Z0-9_]*)\\.(?:[a-zA-Z][a-zA-Z0-9_]*))(?:\\.(\\d+))?".r |
There was a problem hiding this comment.
This might be worth extracting to put at the object level so we only declare it once.
Also, even if the regexp is relatively easy to read, I think a simple comment with an example above the declaration is helpful for regexps.
|
👍 |
| \. # Literal dot. | ||
| (\d+) # Captured shard digits. | ||
| )? # End outer optional noncapturing group for shard. | ||
| """.trim.r // The trim is necessary as (?x) must be at the beginning of the regex. |
There was a problem hiding this comment.
If that's not thorough documentation I don't know what is ! :D
There was a problem hiding this comment.
It'd be a shame if this was only ever used as a private val in the CallCaching endpoint. Can we generalise and extract this regex somewhere more widely useable?
There was a problem hiding this comment.
sure I can put it in an appropriate package.scala
|
👍 after changes. The wheel has chosen @kshakir as the second reviewer. |
| type: string | ||
| in: path | ||
| - name: call | ||
| description: Call FQN |
There was a problem hiding this comment.
This name and description don't match the same parameter in other endpoints, which seem to (mostly) be callFqn and Call fully qualified name.
| security: | ||
| - google_oauth: | ||
| - openid | ||
| '/workflows/{version}/{id}/call-caching/{callFqn}': |
There was a problem hiding this comment.
I collapsed the Spray endpoints per @cjllanwarne's suggestion, but unfortunately optional path parameters are not currently allowed in Swagger:
So while this Swagger YAML and the README.md document two endpoints, in reality Cromwell only implements one endpoint which can handle both the whole workflow and individual call use cases. If a future version of Swagger adds support for optional path parameters we can change these docs to more closely match the underlying reality.
mcovarr
force-pushed
the
mlc_call_caching
branch
from
88275c9
to
6783d72
Compare
|
@cjllanwarne @kshakir I've tried to address all of your comments, please let me know if this looks okay. Thanks! |
|
👍 |
|
👍 |
mcovarr
force-pushed
the
mlc_call_caching
branch
from
54e531c
to
2c354e8
Compare
No description provided.