CLI tool is not replacing '$' placeholders with environment variables

[ekremney]

Describe the bug
CLI tool is not replacing '$' placeholders (indented more than one level) with environment variables.

To Reproduce
Example manifest file:

packages:
  __APP_PACKAGE__:
    version: 1.0.0
    license:  Adobe-2006
    actions:
      someAction:
        function: dist/someAction
        runtime: nodejs:10
        web-export: true
        annotations:
          final: true
        inputs:
          eventingConnection:
            bootstrapServers: $BOOTSTRAP_SERVERS
            saslUsername: $SASL_USERNAME

Expected behavior
I expect aio cli tool to replace $ placeholders with environment variables regardless of the indentation level.

Additional context
An example error:

Error: Error connecting to the host: $BOOTSTRAP_SERVERS failed - getaddrinfo ENOTFOUND

[shazron]

I'm not sure this is valid input.

According to the spec, an action takes an optional inputs key, which takes in a list of parameters: https://github.com/apache/openwhisk-wskdeploy/blob/master/specification/html/spec_actions.md#actions

parameters are specified as: https://github.com/apache/openwhisk-wskdeploy/blob/master/specification/html/spec_parameters.md

in the parameters spec above, there is a sub-key called properties that takes in a list of parameters, but that's different than your case.

It's possible that the spec hasn't been updated, but the tests have been (I encountered this for #138 (comment)).

However, I went through all the test fixtures and none of them had the scenario you had, which makes me believe it is not supported.

[tysonnorris]

does structuring inputs like:

        inputs:
          bootstrapServers: $BOOTSTRAP_SERVERS
          saslUsername: $SASL_USERNAME

work?

[moritzraho]

@tysonnorris yes this is supported, $VAR being an env variable.

What's not supported, is what the issue mentions, i.e. multi level input objects with env vars

[moritzraho]

@shazron in that case should we mark this as an enhancement instead of a bug?
As the openwhisk api seems to support multi level input objects, it would be nice if we would too?

EDIT: more details: what I mean is that as of now we can set object inputs in the manifest (even if we don't officially support it) but $VAR are not replaced in such objects. So my question is should we support input objects officially in which case I feel we should also support the $VAR?

[shazron]

Since we are currently following the wskdeploy manifest format spec, adding this enhancement will break compatibility. We need to decide if compatibility is paramount for our manifest, and to weigh that with any future breaking enhancements.

[meryllblanchet]

Compatibility is as of today a paramount for our manifest and I'm not in favor of deviating from the original specifications unless there's a very good reason and benefit for our end-users.

I propose then to close the issue for the time being and reevaluate if we see this as a recurring need.

[meryllblanchet]

Reopening as @ekremney has some more information to provide

[ekremney]

According to the specification, parameter types include object which is defined as:

The parameter itself is an object with the associated defined Parameters (schemas).

Object type is explained at the bottom of the page

The Object type allows for complex objects to be declared as parameters with an optional validatable schema.

As a developer who uses Openwhisk, I would like to be able to group my parameters so that I can address them differently in my code. It especially comes very handy if you use openwhisk wrappers. Ie:

inputs:
  logging:
    host: <host>
    token: <token>
    index: <index>
  service:
    host: <host>
    token: <token>
...

Then you can just initialize your wrappers:

...
new LoggingWrapper(params.logging);
...

FYI, we are migrating from wskdeploy to aio. wskdeploy tool injects ENV variables into nested parameters.

@meryllblanchet @tysonnorris @shazron

[solaris007]

Also, wskdeploy features sample multi-level config and associated test:

multi-level inputs test file: https://github.com/apache/openwhisk-wskdeploy/blob/master/tests/dat/manifest_validate_multiline_params.yaml

and the test case:
https://github.com/apache/openwhisk-wskdeploy/blob/db5e34e35825900c52017b545e56935c5bea5578/parsers/manifest_parser_test.go#L288

I kindly ask you to add feature parity with wskdeploy in this case. single-level configurations are super ugly and unreadable.

[shazron]

Thanks @ekremney, it is clear now. I didn't see this file. I'll send a PR to the spec_parameters.md doc to link the docs to the type schema at spec_parameter_types.md.

There doesn't seem to be any test fixtures in wskdeploy that test inputs with object types (as I mentioned in a previous comment, all are type string), so I'm surprised it works with wskdeploy (probably just an omission in the tests).

[shazron]

PR filed: apache/openwhisk-wskdeploy#1094

[shazron]

@solaris007 I don't think there is any question that we will try to achieve parity with wskdeploy, we were trying to determine if it is in the spec, which it has been clarified that it is.

With regards to the tests you mentioned, the closest I see is for type json, which is entirely different than type object. There is no test for object in that file, thus no test for "nested objects" (the file tests multi-line params).

[alexkli]

Why not simply replace env vars in all of the manifest.yml in a first pass of the text file itself before parsing the yaml? Makes the code simpler and it works everywhere. If done right you can also do arrays etc. in the replacement.

I don’t know if that will create a conflict with the wskdeploy spec but aio is different already.

Here is my workaround for that using npm scripts:

Install envsub:

npm install --save-dev envsub

In package.json add:

  "scripts": {
    "deploy": "aio app deploy",
    "predeploy": "cp manifest.yml manifest.backup.yml; envsub -f .env -s dollar-basic manifest.yml",
    "postdeploy": "mv manifest.backup.yml manifest.yml"
  }

Then run npm run deploy to invoke aio app deploy. to pass arguments to aio app deploy, run e.g. npm run deploy -- --skip-static.

Note it is slightly dangerous since if deploy fails then it will not run postdeploy and restore the original manifest. So you might have a modified manifest with e.g. credentials present, and could mistakenly commit them. So watch out.

[ekremney]

Hi @alexkli,

This issue just pinpoints the discrepancy between wskdeploy and aio. wskdeploy supports object type parameters whereas aio does not.

That's a nice workaround you mention. In fact, we used envsubst for a while, then we decide to use yaml anchors to group input parameters so that we won't have to repeat inputs for each action while keeping them top-level (for aio to work well). For example:

inputs:
  auth: &auth
    public-key: $PUBLIC_KEY
  db: &db
    host: $DB_HOST
    password: $DB_PASS
  logging: &logging
    host: $LOGGING_HOST
    token: $LOGGING_TOKEN

packages:
  __APP_PACKAGE__:
  actions:
    login:
      function: actions/login
      inputs:
        <<: *auth
        <<: *logging
    getSomething:
      function: actions/getSomething
      inputs:
        <<: *db
        <<: *logging
    doSomething:
      function: actions/doSomething
      inputs:
        <<: *logging

[alexkli]

@ekremney but your setup doesn't create nested input objects, it just ends up being flat. if you really want nested inputs/params in your actions then this doesn't help... or am I missing something?