[GitHub] csantanapr commented on a change in pull request #3689: Enable extending environment variables of Controller and Invoker

2018-05-31 Thread GitBox 
csantanapr commented on a change in pull request #3689: Enable extending 
environment variables of Controller and Invoker
URL: 
https://github.com/apache/incubator-openwhisk/pull/3689#discussion_r192136706
 
 

 ##
 File path: ansible/roles/controller/tasks/deploy.yml
 ##
 @@ -236,6 +236,11 @@
 
   "CONFIG_whisk_transactions_header": "{{ transactions.header }}"
 
+- name: merge extra env variables
+  set_fact:
+controller_env: "{{ controller_env | default({}) | 
combine(controller.extraEnv) }}"
+  when: controller.extraEnv | default(false)
 
 Review comment:
   I see define the default `{}` and then drop the `when` I will give this a 
test and update


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] csantanapr commented on a change in pull request #3689: Enable extending environment variables of Controller and Invoker

2018-05-31 Thread GitBox 
csantanapr commented on a change in pull request #3689: Enable extending 
environment variables of Controller and Invoker
URL: 
https://github.com/apache/incubator-openwhisk/pull/3689#discussion_r192136706
 
 

 ##
 File path: ansible/roles/controller/tasks/deploy.yml
 ##
 @@ -236,6 +236,11 @@
 
   "CONFIG_whisk_transactions_header": "{{ transactions.header }}"
 
+- name: merge extra env variables
+  set_fact:
+controller_env: "{{ controller_env | default({}) | 
combine(controller.extraEnv) }}"
+  when: controller.extraEnv | default(false)
 
 Review comment:
   I see define the default and then drop the `when` I will give this a test 
and update


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] csantanapr commented on a change in pull request #3689: Enable extending environment variables of Controller and Invoker

2018-05-31 Thread GitBox 
csantanapr commented on a change in pull request #3689: Enable extending 
environment variables of Controller and Invoker
URL: 
https://github.com/apache/incubator-openwhisk/pull/3689#discussion_r192131671
 
 

 ##
 File path: ansible/roles/controller/tasks/deploy.yml
 ##
 @@ -236,6 +236,11 @@
 
   "CONFIG_whisk_transactions_header": "{{ transactions.header }}"
 
+- name: merge extra env variables
+  set_fact:
+controller_env: "{{ controller_env | default({}) | 
combine(controller.extraEnv) }}"
+  when: controller.extraEnv | default(false)
 
 Review comment:
   I assumed we need it the extra safety, your proposing just dropping the 
`when` line?
since in group_vars we already default to `default({})` and extra check is 
not need it.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] csantanapr commented on a change in pull request #3689: Enable extending environment variables of Controller and Invoker

2018-05-23 Thread GitBox 
csantanapr commented on a change in pull request #3689: Enable extending 
environment variables of Controller and Invoker
URL: 
https://github.com/apache/incubator-openwhisk/pull/3689#discussion_r190335870
 
 

 ##
 File path: ansible/environments/local/group_vars/all
 ##
 @@ -2,6 +2,7 @@
 # license agreements; and to You under the Apache License, Version 2.0.
 
 whisk_version_name: local
+extraEnvSets: "{{ lookup('env', 'OPENWHISK_EXTRA_ENV_SETS')|default(false, 
true) }}"
 
 Review comment:
   We should not assume that is one top level set and force on both controller 
and invoker
   Maybe user wants to set certain config or secret that is intended for 
controller but not invoker.
   Or you want to set the same variable ie apikey to the db and is different 
for both controller and invoker.
   
   What about having namespace with option to override
   ```
   invoker:
  extraEnv: "{{ invoker_extra_env | lookup('env', 'OPENWHISK_EXTRA_ENV') | 
default(false, true) }}"
   
   controller:
  extraEnv: "{{ controller_extra_env | lookup('env', 'OPENWHISK_EXTRA_ENV') 
| default(false, true) }}"
   ```


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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