[GitHub] [incubator-openwhisk] dgrove-oss commented on a change in pull request #4570: KCF: propagate cf_ca_extraArgs_env_i into user containers
dgrove-oss commented on a change in pull request #4570: KCF: propagate
cf_ca_extraArgs_env_i into user containers
URL:
https://github.com/apache/incubator-openwhisk/pull/4570#discussion_r309316353
##
File path: core/invoker/src/main/resources/application.conf
##
@@ -93,6 +93,7 @@ whisk {
dns-servers: []
dns-search: []
dns-options: []
+ extra-env-vars: [] # sequence of `key` and/or `key=value` bindings to
add to all user action container environments
Review comment:
I prototyped using a map instead of a seq[k=v] and found an unfortunate
interaction with `transformEnvironment.sh` that makes a map cumbersome to use.
The problem is `transformEnvirovnment.sh` rewrites `_` to `.` which makes it
quite cumbersome to define environment variables whose name contains an `_`
without adding an escape sequence.
For example, consider trying to set `__OW_ALLOW_CONCURRENT=false` via a
`CONFIG_` envar. With a Seq, it is:
```
CONFIG_whisk_containerFactory_containerArgs_extraArgs_env_0=__OW_ALLOW_CONCURRENT=false
```
With a Map, it should be
```
CONFIG_whisk_containerFactory_containerArgs_extraArgs_env___OW_ALLOW_CONCURRENT=false
```
but this doesn't work because we also need to somehow escape all the `_`
after `env_` to prevent `transformEnvironment.sh` from converting them to `.`
(which would take an extension to `transformEnvironment.sh` and proper
documentation of it).
So, I think using Seq to avoid needing to escape `_` is a better trade-off
overall.
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] [incubator-openwhisk] dgrove-oss commented on a change in pull request #4570: KCF: propagate cf_ca_extraArgs_env_i into user containers
dgrove-oss commented on a change in pull request #4570: KCF: propagate
cf_ca_extraArgs_env_i into user containers
URL:
https://github.com/apache/incubator-openwhisk/pull/4570#discussion_r309316353
##
File path: core/invoker/src/main/resources/application.conf
##
@@ -93,6 +93,7 @@ whisk {
dns-servers: []
dns-search: []
dns-options: []
+ extra-env-vars: [] # sequence of `key` and/or `key=value` bindings to
add to all user action container environments
Review comment:
I prototyped using a map instead of a seq[k=v] and found an unfortunate
interaction with `transformEnvironment.sh` that makes a map cumbersome to use.
The problem is `transformEnvirovnment.sh` rewrites `_` to `.` which makes it
quite cumbersome to define environment variables whose name contains an `_`
without adding an escape sequence.
For example, consider trying to set `__OW_ALLOW_CONCURRENT=false` via a
`CONFIG_` envar. With a Seq, it is:
```
CONFIG_whisk_containerFactory_containerArgs_extraArgs_env_0=__OW_ALLOW_CONCURRENT=false
```
With a Map, it would be
```
CONFIG_whisk_containerFactory_containerArgs_extraArgs_env___OW_ALLOW_CONCURRENT=false
```
but we also need to somehow escape all the `_` after `env_` to prevent
`transformEnvironment.sh` from converting them to `.`.
So, I think using Seq to avoid needing to escape `_` is a better trade-off
overall.
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] [incubator-openwhisk] dgrove-oss commented on a change in pull request #4570: KCF: propagate cf_ca_extraArgs_env_i into user containers
dgrove-oss commented on a change in pull request #4570: KCF: propagate
cf_ca_extraArgs_env_i into user containers
URL:
https://github.com/apache/incubator-openwhisk/pull/4570#discussion_r308188978
##
File path: core/invoker/src/main/resources/application.conf
##
@@ -93,6 +93,7 @@ whisk {
dns-servers: []
dns-search: []
dns-options: []
+ extra-env-vars: [] # sequence of `key` and/or `key=value` bindings to
add to all user action container environments
Review comment:
reasonable point. I'm on vacation until Wed; will make the change when I
get back.
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] [incubator-openwhisk] dgrove-oss commented on a change in pull request #4570: KCF: propagate cf_ca_extraArgs_env_i into user containers
dgrove-oss commented on a change in pull request #4570: KCF: propagate
cf_ca_extraArgs_env_i into user containers
URL:
https://github.com/apache/incubator-openwhisk/pull/4570#discussion_r307853035
##
File path:
common/scala/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerFactory.scala
##
@@ -30,7 +30,18 @@ case class ContainerArgsConfig(network: String,
dnsServers: Seq[String] = Seq.empty,
dnsSearch: Seq[String] = Seq.empty,
dnsOptions: Seq[String] = Seq.empty,
- extraArgs: Map[String, Set[String]] = Map.empty)
+ extraEnvVars: Seq[String] = Seq.empty,
+ extraArgs: Map[String, Set[String]] =
Map.empty) {
+
+ val extraEnvVarMap: Map[String, String] =
+extraEnvVars.flatMap {
+ _.split("=", 2) match {
+case Array(key)=> Some(key -> "")
+case Array(key, value) => Some(key -> value)
+case _ => None
Review comment:
I think the _ case is unreachable and maybe we don't need the Option type
now since a failure to split on `=` can be interpreted as a request to simply
set an envvar with an empty value. But I don't think it hurts to do it this
way.
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
