[GitHub] [openwhisk] duynguyen commented on a change in pull request #4503: Add optional config for appending custom registry to user provided images

2019-09-20 Thread GitBox 
duynguyen commented on a change in pull request #4503: Add optional config for 
appending custom registry to user provided images
URL: https://github.com/apache/openwhisk/pull/4503#discussion_r326551419
 
 

 ##
 File path: ansible/group_vars/all
 ##
 @@ -55,7 +55,8 @@ whisk:
 # configuration parameters related to support runtimes (see 
org.apache.openwhisk.core.entity.ExecManifest for schema of the manifest).
 # briefly the parameters are:
 #
-#   runtimes_registry: optional registry (with trailing slack) where to pull 
docker images from for runtimes and backbox images
+#   runtimes_registry: optional registry (with trailing slash) where to pull 
docker images from for default runtimes (in manifest)
+#   user_images_registry: optional registry (with trailing slash) where to 
pull docker images from for blackbox images
 
 Review comment:
   They are separately evaluated.
   Earlier there was only one option to add `runtimes_registry` to default 
images. Blackbox images were only evaluated as is (no system registry applied).
   This PR adds `user_images_registry` to apply blackbox images. If this is not 
set, blackbox images are evaluated by its own name.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [openwhisk] duynguyen commented on a change in pull request #4503: Add optional config for appending custom registry to user provided images

2019-09-18 Thread GitBox 
duynguyen commented on a change in pull request #4503: Add optional config for 
appending custom registry to user provided images
URL: https://github.com/apache/openwhisk/pull/4503#discussion_r325750386
 
 

 ##
 File path: 
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/docker/DockerContainerFactory.scala
 ##
 @@ -60,10 +62,14 @@ class DockerContainerFactory(instance: InvokerInstanceId,
userProvidedImage: Boolean,
memory: ByteSize,
cpuShares: Int)(implicit config: WhiskConfig, 
logging: Logging): Future[Container] = {
+val registryConfig =
+  ContainerFactory.resolveRegisterConfig(userProvidedImage, 
runtimesRegistryConfig, userImagesRegistryConfig)
+val image =
+  if (userProvidedImage && registryConfig.url.isEmpty) Left(actionImage)
+  else Right(actionImage.localImageName(registryConfig.url))
 
 Review comment:
   ok, I think what you said makes sense. thanks @sven-lange-last.
   Please see the latest PR version.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [openwhisk] duynguyen commented on a change in pull request #4503: Add optional config for appending custom registry to user provided images

2019-09-17 Thread GitBox 
duynguyen commented on a change in pull request #4503: Add optional config for 
appending custom registry to user provided images
URL: https://github.com/apache/openwhisk/pull/4503#discussion_r325299534
 
 

 ##
 File path: 
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/docker/DockerContainerFactory.scala
 ##
 @@ -60,10 +62,14 @@ class DockerContainerFactory(instance: InvokerInstanceId,
userProvidedImage: Boolean,
memory: ByteSize,
cpuShares: Int)(implicit config: WhiskConfig, 
logging: Logging): Future[Container] = {
+val registryConfig =
+  ContainerFactory.resolveRegisterConfig(userProvidedImage, 
runtimesRegistryConfig, userImagesRegistryConfig)
+val image =
+  if (userProvidedImage && registryConfig.url.isEmpty) Left(actionImage)
+  else Right(actionImage.localImageName(registryConfig.url))
 
 Review comment:
   I guess you meant it to be:
   ```
   val registryConfig =
 ContainerFactory.resolveRegisterConfig(userProvidedImage, 
runtimesRegistryConfig, userImagesRegistryConfig)
   val imageRef = actionImage.localImageName(registryConfig.url)
   val image = if (userProvidedImage) Left(actionImage) else Right(imageRef)
   ```
   (`actionImage` in Left, not `imageRef`)
   
   However the condition is not the same as before anymore. In the case of 
`userProvidedImage==true`, the raw `imageName` object is passed into 
DockerContainer, which will pull the "public image name" (without custom 
blackbox registry), see 
[DockerContainer.scala#L105](https://github.com/apache/openwhisk/blob/master/core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/docker/DockerContainer.scala#L105).
 Hence I added a condition for registryConfig being empty or not.
   
   In general this flow does not fit well into the existing flow of 
DockerContainerFactory. I am working on an enhancement to get rid of 
`publicImageName`, make `localImageName` (to be renamed) a single image name 
resolver, and apply it throughout the code base. I will push that change for 
review.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [openwhisk] duynguyen commented on a change in pull request #4503: Add optional config for appending custom registry to user provided images

2019-09-17 Thread GitBox 
duynguyen commented on a change in pull request #4503: Add optional config for 
appending custom registry to user provided images
URL: https://github.com/apache/openwhisk/pull/4503#discussion_r325250701
 
 

 ##
 File path: 
common/scala/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerFactory.scala
 ##
 @@ -111,6 +111,20 @@ object ContainerFactory {
   /** include the instance name, if specified and strip invalid chars before 
attempting to use them in the container name */
   def containerNamePrefix(instanceId: InvokerInstanceId): String =
 
s"wsk${instanceId.uniqueName.getOrElse("")}${instanceId.toInt}".filter(isAllowed)
+
+  def resolveRegisterConfig(userProvidedImage: Boolean,
+runtimesRegistryConfig: RuntimesRegistryConfig,
+userImagesRegistryConfig: RuntimesRegistryConfig): 
RuntimesRegistryConfig = {
 
 Review comment:
   yes that makes sense. In fact that was a typo with `resolveRegisterConfig ` 
...
   thanks for pointing out.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [openwhisk] duynguyen commented on a change in pull request #4503: Add optional config for appending custom registry to user provided images

2019-09-05 Thread GitBox 
duynguyen commented on a change in pull request #4503: Add optional config for 
appending custom registry to user provided images
URL: https://github.com/apache/openwhisk/pull/4503#discussion_r321240007
 
 

 ##
 File path: tests/src/test/resources/application.conf.j2
 ##
 @@ -86,6 +86,7 @@ whisk {
 
 container-factory.runtimes-registry {
 url = "{{ runtimes_registry | default('') }}"
+include-user-images = "{{ runtimes_registry_user_images | 
default('false') }}"
 
 Review comment:
   thanks for pointing out, removed the outdated boolean flag and added the 
string config.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services