Treat blackbox action code as attachment #3979

chetanmeh · 2018-08-21T15:30:29Z

Treats blackbox action code as attachment

Description

This is next installment in the attachment saga (previous in this series #3945). With this change code related to blackbox action would also be stored as attachment.

Change is backward compatible in reading code
Upon update or new action being created the code would be stored as attachment

Related issue and scope

I opened an issue to propose and discuss this change (Store action code as attachments #3944)

My changes affect the following components

Types of changes

Bug fix (generally a non-breaking change which closes an issue).
Enhancement or new feature (adds new functionality).
Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

I signed an Apache CLA.
I reviewed the style guides and followed the recommendations (Travis CI will check :).
I added tests to cover my changes.
My changes require further changes to the documentation.
I updated the documentation where necessary.

codecov-io · 2018-08-22T08:15:09Z

Codecov Report

Merging #3979 into master will decrease coverage by 4.58%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master    #3979      +/-   ##
==========================================
- Coverage   85.82%   81.24%   -4.59%     
==========================================
  Files         146      146              
  Lines        7084     7095      +11     
  Branches      440      443       +3     
==========================================
- Hits         6080     5764     -316     
- Misses       1004     1331     +327

Impacted Files	Coverage Δ
...src/main/scala/whisk/core/entity/Attachments.scala	`90.47% <100%> (+1.58%)`	⬆️
.../scala/src/main/scala/whisk/core/entity/Exec.scala	`82.23% <100%> (+0.53%)`	⬆️
...src/main/scala/whisk/core/entity/WhiskAction.scala	`86.25% <90%> (+0.15%)`	⬆️
...core/database/cosmosdb/RxObservableImplicits.scala	`0% <0%> (-100%)`	⬇️
...core/database/cosmosdb/CosmosDBArtifactStore.scala	`0% <0%> (-95.1%)`	⬇️
...sk/core/database/cosmosdb/CosmosDBViewMapper.scala	`0% <0%> (-92.6%)`	⬇️
...whisk/core/database/cosmosdb/CosmosDBSupport.scala	`0% <0%> (-81.82%)`	⬇️
...abase/cosmosdb/CosmosDBArtifactStoreProvider.scala	`0% <0%> (-58.83%)`	⬇️
.../scala/src/main/scala/whisk/core/entity/Size.scala	`89.47% <0%> (-5.27%)`	⬇️
...la/whisk/core/database/cosmosdb/CosmosDBUtil.scala	`92% <0%> (-4%)`	⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cc3832...00473fa. Read the comment docs.

chetanmeh · 2018-08-22T08:21:27Z

This PR is now ready for review

chetanmeh · 2018-08-24T09:24:47Z

Rebased to. resolve the conflict. @dubee Would you be able to review this PR?

markusthoemmes

Greatly crafted refactorings and enhancement! 🎉

The tests look solid to me and seem to address any migration paths there might be. It also seems like this is roughly the same change as needed when making all actions attachement based and since the migration logic worked there, I'm quite confident it will work here as well.

A second pair of 👀 will make sense though, as I'm not very proficient in this area.

Great job @chetanmeh, very much appreciated 🎉

markusthoemmes · 2018-08-29T08:39:56Z

tests/src/test/scala/whisk/core/entity/test/ExecTests.scala

+                 |}""".stripMargin.parseJson.asJsObject
+    val action = WhiskAction.serdes.read(json)
+    action.exec should matchPattern { case BlackBoxExec(_, Some(Inline("foo")), None, false, false) => }
+  }


Thanks for testing this!

markusthoemmes · 2018-08-29T08:40:05Z

tests/src/test/scala/whisk/core/entity/test/ExecTests.scala

+                |  }
+                |}""".stripMargin.parseJson.asJsObject
+    js shouldBe js2
+  }


Nice tests, I like 👍

markusthoemmes · 2018-08-29T09:34:42Z

PG2 3553 👍

rabbah

LGTM.

rabbah · 2018-08-29T13:18:16Z

common/scala/src/main/scala/whisk/core/entity/Exec.scala

+
+  private def isBinary(code: JsValue, obj: JsObject): Boolean = {
+    code match {
+      case JsString(c) => isBinaryCode(c)


i know you preserved the previous behavior... but should be check for the presence of the binary field first and only if it's missing (indicating an old schema), compute the binary bit?

Not sure on that - Current read flow would happen for 2 cases

Deserializing the json obtained from ArtifactStore where the binary flag would have been set at time of serialization

Deserializing the user providing json

Would it fine to trust the binary field if provided by user? Probably yes ... but wanted to check on that

rabbah · 2018-08-29T13:23:03Z

tests/src/test/scala/whisk/core/database/test/AttachmentCompatibilityTests.scala

+    val exec = """{
+                 |  "kind": "blackbox",
+                 |  "image": "docker-custom.com/openwhisk-runtime/magic/nodejs:0.0.1",
+                 |  "code":  "while (true)",


do we need another test where this is b64 encoded?

rabbah · 2018-08-29T13:24:03Z

tests/src/test/scala/whisk/core/entity/test/ExecTests.scala

+    val j1 = """{
+               |    "kind": "blackbox",
+               |    "image": "docker-custom.com/openwhisk-runtime/magic/nodejs:0.0.1",
+               |    "code": "SGVsbG8gT3BlbldoaXNr",


ok you have it here (the b64 test).

rabbah · 2018-08-29T13:24:48Z

tests/src/test/scala/whisk/core/entity/test/ExecTests.scala

+               |    "kind": "blackbox",
+               |    "image": "docker-custom.com/openwhisk-runtime/magic/nodejs:0.0.1",
+               |    "code": "SGVsbG8gT3BlbldoaXNr",
+               |    "binary": false


note binary: false should never be stored in the repo incorrectly (since the system always computes it) --- note this is relevant for my comment above about checking if the binary field exists first.

* Use attachments with BlackBoxExec * Simplify code to avoid code duplication * Test for blackbox serialization * Test for cache for blackbox action

chetanmeh added the wip label Aug 21, 2018

chetanmeh self-assigned this Aug 21, 2018

chetanmeh removed the wip label Aug 22, 2018

chetanmeh requested a review from dubee August 22, 2018 08:20

chetanmeh mentioned this pull request Aug 22, 2018

Store action code as attachments #3944

Open

4 tasks

chetanmeh force-pushed the bbox-attachment branch from 0d8affa to 0e30208 Compare August 24, 2018 09:24

markusthoemmes assigned dubee and unassigned chetanmeh Aug 24, 2018

chetanmeh added 7 commits August 29, 2018 12:02

Use attachments with BlackBoxExec

534eece

Simplify code to avoid code duplication

a34e0fe

Fix test expectation

0217730

Test for blackbox serialization

d41d614

Compat test for blackbox action

6267121

Test for cache for blackbox action

23e0d7c

Remove unused import

3c6084c

chetanmeh force-pushed the bbox-attachment branch from 5f52cf0 to 3c6084c Compare August 29, 2018 06:32

markusthoemmes approved these changes Aug 29, 2018

View reviewed changes

Drop println

00473fa

rabbah reviewed Aug 29, 2018

View reviewed changes

dubee approved these changes Aug 29, 2018

View reviewed changes

dubee merged commit fcd0e4b into apache:master Aug 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Treat blackbox action code as attachment #3979

Treat blackbox action code as attachment #3979

chetanmeh commented Aug 21, 2018

codecov-io commented Aug 22, 2018 •

edited

Loading

chetanmeh commented Aug 22, 2018

chetanmeh commented Aug 24, 2018

markusthoemmes left a comment

markusthoemmes Aug 29, 2018

markusthoemmes Aug 29, 2018

markusthoemmes commented Aug 29, 2018

rabbah left a comment

rabbah Aug 29, 2018

chetanmeh Aug 29, 2018

rabbah Aug 29, 2018

rabbah Aug 29, 2018

rabbah Aug 29, 2018

Treat blackbox action code as attachment #3979

Treat blackbox action code as attachment #3979

Conversation

chetanmeh commented Aug 21, 2018

Description

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

codecov-io commented Aug 22, 2018 • edited Loading

Codecov Report

chetanmeh commented Aug 22, 2018

chetanmeh commented Aug 24, 2018

markusthoemmes left a comment

Choose a reason for hiding this comment

markusthoemmes Aug 29, 2018

Choose a reason for hiding this comment

markusthoemmes Aug 29, 2018

Choose a reason for hiding this comment

markusthoemmes commented Aug 29, 2018

rabbah left a comment

Choose a reason for hiding this comment

rabbah Aug 29, 2018

Choose a reason for hiding this comment

chetanmeh Aug 29, 2018

Choose a reason for hiding this comment

rabbah Aug 29, 2018

Choose a reason for hiding this comment

rabbah Aug 29, 2018

Choose a reason for hiding this comment

rabbah Aug 29, 2018

Choose a reason for hiding this comment

codecov-io commented Aug 22, 2018 •

edited

Loading