-
Notifications
You must be signed in to change notification settings - Fork 359
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
BT-684 Initial Blob Storage Impl #6810
Conversation
filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala
Outdated
Show resolved
Hide resolved
filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a first pass of review, this looks great!! I'll try to come back later and look more closely for Scala style stuff as well.
filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala
Outdated
Show resolved
Hide resolved
filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala
Show resolved
Hide resolved
filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala
Outdated
Show resolved
Hide resolved
filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala
Outdated
Show resolved
Hide resolved
filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala
Show resolved
Hide resolved
filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala
Outdated
Show resolved
Hide resolved
filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala
Show resolved
Hide resolved
filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilderFactory.scala
Outdated
Show resolved
Hide resolved
filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala
Show resolved
Hide resolved
filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala
Outdated
Show resolved
Hide resolved
filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala
Outdated
Show resolved
Hide resolved
filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala
Show resolved
Hide resolved
filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilderFactory.scala
Outdated
Show resolved
Hide resolved
filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I'm sure we'll continue tweaking this over time, no reason not to go ahead and merge v1!
Should be OK to go ahead and merge without the code coverage approval - it reflects the tests that actually ran on this PR, so it's finding none. If there's a way to only ignore the test we expect to fail, might be nice, but not a blocker for this PR IMO. |
* pathToFile -> test/testFile.wdl | ||
* | ||
* If the configured container and storage account do not match, the string is considered unparsable | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
No description provided.