Use ‘docker.host’ java property as a way to set Docker access for Runtime unit tests #3616

jonpspri · 2018-05-05T22:17:53Z

Description

These changes allow new scripts using docker-gradle-plugin to use Java properties to pass Docker URL information to the test, rather than inferring it based on whisk. properties. If the ‘docker.host’ java property is not set, the original ‘whisk.properties’ logic is used as a fallback.

Related issue and scope

I opened an issue to propose and discuss this change (#????)

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.

jonpspri · 2018-05-05T22:19:32Z

@csantanapr Please review. If we do just this, we can avoid having to deploy full OpenWhisk when we build/test runtimes.

csantanapr · 2018-05-06T00:43:31Z

@jonpspri check this PR, travis is has errors.

In addition I tried this PR locally, in docker for Mac, where it's fine to run docker run with no host --host specified

I deleted the whisk.properties and try to run the test for the runtime-nodejs runtime

😄  $ rm $OPENWHISK_HOME/whisk.properties
~/dev/whisk/git/apache/incubator-openwhisk-runtime-nodejs (master)
🔥  $ cat OPENWHISK_HOME/whisk.properties
cat: $OPENWHISK_HOME/whisk.properties: No such file or directory
~/dev/whisk/git/apache/incubator-openwhisk-runtime-nodejs (master)
🍰  $ ./gradlew tests:test

> Task :tests:compileTestScala
Pruning sources from previous analysis, due to incompatible CompileSetup.

> Task :tests:test

runtime.actionContainers.NodeJs6ActionContainerTests STANDARD_OUT
    [2018-05-05T20:36:50.788Z] [INFO] Slf4jLogger started

runtime.actionContainers.NodeJs6ActionContainerTests > nodejs6action should run and report an error for script not returning a json object FAILED
    java.lang.AssertionError: '/Users/csantana23/dev/whisk/git/apache/incubator-openwhisk/whisk.properties' does not exists but required

runtime.actionContainers.NodeJs6ActionContainerTests > nodejs6action should run a node script FAILED
    java.lang.NoClassDefFoundError: Could not initialize class common.WhiskProperties

runtime.actionContainers.NodeJs8ActionContainerTests STANDARD_OUT
    [2018-05-05T20:36:51.069Z] [INFO] Slf4jLogger started

runtime.actionContainers.NodeJs8ActionContainerTests > action-nodejs-v8 should run and report an error for script not returning a json object FAILED
    java.lang.NoClassDefFoundError: Could not initialize class common.WhiskProperties

3 tests completed, 3 failed

It looks like something still depends on whisk.properties

csantanapr · 2018-05-06T00:53:53Z

Ah it looks like just having one property is enough

echo whisk.version.name=local > $OPENWHISK_HOME/whisk.properties

But now fails the same as Travis

runtime.actionContainers.NodeJs8ActionContainerTests > action-nodejs-v8 should support async and await FAILED
    java.lang.AssertionError: assertion failed: 'docker run' did not exit with 0: (125,,unknown shorthand flag: 'p' in -p
    See 'docker --help'.

csantanapr · 2018-05-06T01:10:45Z

tests/src/test/scala/actionContainers/ActionContainer.scala

-      " "
+      val version = WhiskProperties.getProperty("whisk.version.name")
+      // Check if we are running on docker-machine env.
+      val hostStr = if (version.toLowerCase().contains("mac")) {


remove val hostStr

if (version.toLowerCase().contains("mac")) { s" --host tcp://${WhiskProperties.getMainDockerEndpoint()} " } else { " " }

Oops. Yep, that’ll help.

jonpspri · 2018-05-06T11:55:37Z

Apologies for the spent time. I should have run this through the cases for whisk.properties still being there more thoroughly. I’ll ping you when I get a clean Travis build.

codecov-io · 2018-05-06T12:44:57Z

Codecov Report

Merging #3616 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3616   +/-   ##
=======================================
  Coverage   74.49%   74.49%           
=======================================
  Files         126      126           
  Lines        5990     5990           
  Branches      390      390           
=======================================
  Hits         4462     4462           
  Misses       1528     1528

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 403d341...fa44248. Read the comment docs.

csantanapr · 2018-05-06T13:00:43Z

tests/src/test/scala/actionContainers/ActionContainer.scala

-      " "
+      val version = WhiskProperties.getProperty("whisk.version.name")
+      // Check if we are running on docker-machine env.
+      if (version.toLowerCase().contains("mac")) {


What do you think making the unit tests work with no “whisk.properties” present at all?
By adding a check if System.getenv("OPENWHISK_HOME")/WhiskProperties.WHISK_PROPS_FILE doesn’t exist.

Same thing below
Check if file doesn’t exist and onOSX the. Use a port
Or if file exist read if local and OSX then same as today use a port

If file doesn’t exist and not OSX then don’t use a port

Could be. I was thinking this morning of stringing together several options and using whichever came back first with an affirmative result:

1). The ‘docker.host’ Java property (which is what I’ll be using in multi-arch builds)
2). The ‘DOCKER_HOST’ environment variable (which is set on docker-machine systems)
3). The ‘WhiskProperties’ logic if ‘WhiskProperties’ exists (add a try/catch) block
4). Just run ‘docker info’ and see if it works
5). Fail

I can foldLeft an ordered list of anonymous functions implementing each of those to make it fairly maintainable. Of course, it’ll take us some time to test the tests...

Yeah sounds good

Okay. It'll take a little time to do and test. I'll mark this WIP for now.

csantanapr · 2018-05-09T20:13:37Z

@jonpspri your done with WIP and ready for review?

csantanapr · 2018-05-10T00:30:25Z

PG2 3111 ⌛️

csantanapr · 2018-05-10T03:14:01Z

PG2 3111 🔵

csantanapr · 2018-05-10T21:31:19Z

New PG1 2889 ⌛️

…-options-4-test

dubee · 2018-05-11T16:13:37Z

tests/src/test/scala/actionContainers/ActionContainer.scala

@@ -145,8 +169,7 @@ object ActionContainer {

    // ...find out its IP address...
    val (ip, port) =
-      if (WhiskProperties.getProperty("whisk.version.name") == "local" &&
-          WhiskProperties.onMacOSX()) {
+      if (System.getProperty("os.name").toLowerCase().contains("mac")) {


@csantanapr, said to check if DOCKER_HOST is set here to determine if docker-machine is being used on a Mac.

…-options-4-test

csantanapr · 2018-05-15T03:01:45Z

@dubee this is ready now.
left docker-machine the way it is today unchanged, on docker-machine DOCKER_HOST is always defined.

dubee

Run ./gradlew scalafmtAll. Looks like the Travis check is failing.

dubee · 2018-05-15T19:20:23Z

tests/src/test/scala/actionContainers/ActionContainer.scala

+    // Test here that this actually works, otherwise throw a somewhat understandable error message
+    proc(s"$dockerCmdString info").onComplete {
+      case Success((v, _, _)) if (v != 0) =>
+        throw new RuntimeException("""Unable to connect to docker host using '$d' as command string.


What is $d?

I guess it needs to be $dockerCmdString

Add s in front of the string as well.

s"""Unable to connect to docker host using '$dockerCmdString' as command string. |The docker host is determined using the Java property 'docker.host' or |the envirnoment variable 'DOCKER_HOST'. Please verify that one or the |other is set for your build/test process.""".stripMargin

csantanapr · 2018-05-15T19:38:20Z

PG3/2269 🏃

csantanapr · 2018-05-15T22:18:50Z

PG3/2269 🔵

…time unit tests (apache#3616)

csantanapr self-requested a review May 6, 2018 00:35

csantanapr self-assigned this May 6, 2018

csantanapr requested changes May 6, 2018

View reviewed changes

Pick out Docker container w/o whisk.properties

dd3bc04

jonpspri force-pushed the docker-connection-options-4-test branch from 7549d44 to dd3bc04 Compare May 6, 2018 11:59

csantanapr reviewed May 6, 2018

View reviewed changes

jonpspri changed the title ~~Use ‘docker.host’ java property as a way to set Docker access for Runtime unit tests~~ WIP: Use ‘docker.host’ java property as a way to set Docker access for Runtime unit tests May 7, 2018

Refactor docker Url detection code

d398103

jonpspri force-pushed the docker-connection-options-4-test branch from 90506cf to d398103 Compare May 7, 2018 22:12

Scalafmt

1e19adf

csantanapr approved these changes May 10, 2018

View reviewed changes

remove dependency on common.WhiskProperties

6f4c9f2

csantanapr changed the title ~~WIP: Use ‘docker.host’ java property as a way to set Docker access for Runtime unit tests~~ Use ‘docker.host’ java property as a way to set Docker access for Runtime unit tests May 10, 2018

Merge remote-tracking branch 'upstream/master' into docker-connection…

f53e4d3

…-options-4-test

dubee requested changes May 11, 2018

View reviewed changes

csantanapr added 2 commits May 14, 2018 22:55

leaving docker-machine untouch

b275e55

Merge remote-tracking branch 'upstream/master' into docker-connection…

eaf60cc

…-options-4-test

dubee requested changes May 15, 2018

View reviewed changes

fix scalafmt

c9b1bb4

dubee requested changes May 15, 2018

View reviewed changes

fix exception message

fa44248

dubee approved these changes May 15, 2018

View reviewed changes

csantanapr merged commit 8e7fc3e into apache:master May 15, 2018

BillZong pushed a commit to BillZong/openwhisk that referenced this pull request Nov 18, 2019

Use ‘docker.host’ java property as a way to set Docker access for Run…

93bab46

…time unit tests (apache#3616)

jonpspri deleted the docker-connection-options-4-test branch April 19, 2020 11:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ‘docker.host’ java property as a way to set Docker access for Runtime unit tests #3616

Use ‘docker.host’ java property as a way to set Docker access for Runtime unit tests #3616

jonpspri commented May 5, 2018

jonpspri commented May 5, 2018

csantanapr commented May 6, 2018 •

edited

csantanapr commented May 6, 2018

csantanapr May 6, 2018

jonpspri May 6, 2018

jonpspri commented May 6, 2018

codecov-io commented May 6, 2018 •

edited

csantanapr May 6, 2018

csantanapr May 6, 2018

csantanapr May 6, 2018

jonpspri May 6, 2018

csantanapr May 6, 2018

jonpspri May 7, 2018

csantanapr commented May 9, 2018

csantanapr commented May 10, 2018

csantanapr commented May 10, 2018

csantanapr commented May 10, 2018

dubee May 11, 2018

csantanapr commented May 15, 2018

dubee left a comment

dubee May 15, 2018

csantanapr May 15, 2018

dubee May 15, 2018 •

edited

csantanapr commented May 15, 2018

csantanapr commented May 15, 2018

Use ‘docker.host’ java property as a way to set Docker access for Runtime unit tests #3616

Use ‘docker.host’ java property as a way to set Docker access for Runtime unit tests #3616

Conversation

jonpspri commented May 5, 2018

Description

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

jonpspri commented May 5, 2018

csantanapr commented May 6, 2018 • edited

csantanapr commented May 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonpspri commented May 6, 2018

codecov-io commented May 6, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csantanapr commented May 9, 2018

csantanapr commented May 10, 2018

csantanapr commented May 10, 2018

csantanapr commented May 10, 2018

Choose a reason for hiding this comment

csantanapr commented May 15, 2018

dubee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dubee May 15, 2018 • edited

Choose a reason for hiding this comment

csantanapr commented May 15, 2018

csantanapr commented May 15, 2018

csantanapr commented May 6, 2018 •

edited

codecov-io commented May 6, 2018 •

edited

dubee May 15, 2018 •

edited