awslabs / aws-shell

An integrated shell for working with the AWS CLI.
Apache License 2.0
7.19k stars 773 forks source link

Drop key from environment params if None #146

Closed joguSD closed 8 years ago

joguSD commented 8 years ago

When the environment parameters for a request are being resolved if the query to get the variable returns None it will be dropped from the request. This will allow for optional parameters without having to have a request stage for each permutation of parameters.

kyleknap commented 8 years ago

Not too sure if this can be defined in your schema but I wonder if it makes sense to add the notion of an optional parameter in your definition schema? I ask this because silent errors are annoying in the sense that if you have defined an improper resolution path and something goes wrong later in the process, you will have to trace through the entire process to see that you messed up a resolution path. So explicit error throwing early on in the process is much more helpful. What is your thoughts on this?

joguSD commented 8 years ago

Right now parameter resolution is very simple and an extension could be made to have more complicated resolutions. At present, a sample EnvParameters might look like:

{
     "InstanceId": "JMESPath.query"
}

Which would apply the query on the environment and set that result as the new value to be used when actually calling the operation. The assumption here is that the values for each key are going to be strings that represents a query.

The following extension could be made:

{
    "InstanceId": {
        "Type": "Query", 
        "Path": "JMESPath.query",
        "Required": true
    }
}

However, the above would only work in the way parameter resolution is currently defined. To extend the conversation something like the following is currently impossible:

{
    "Subnet": {
          "FieldOne": "Query.One",
          "FieldTwo": "Query.Two"
    }
}

This would fail as the behavior for resolving a key when it's value is anything other than a string (JMESPath query) is undefined. I was considering adding a feature that resolution would recurse when a dict or list is found resolving all queries as they're found. The ability to specify an optional field wouldn't be compatible with this as far as I can tell. Perhaps you could do something like this if you wanted to support recursion in addition to optional types, etc:

{
    "Subnet": {
        "Type": "Recurse",
        "Parameters": {
             "FieldOne": "Query.One",
             "FieldTwo": "Query.Two"
        }
    }
}

Which would resolve to:

{
    "Subnet": {
        "FieldOne": "ResultofQueryOne",
        "FieldTwo": "ResultOfQueryTwo"
    }
}

I feel that extending the specification for variable resolution is a separate issue that needs to be discussed. For now with the simple resolution, dropping keys when they are None makes the system much more usable and is necessary for any wizard that has more than one optional parameter.

kyleknap commented 8 years ago

Interesting. Yeah seems would complicate the schema more than needed especially since optional parameters is not really needed yet. In that case, we should at least log that we missed a variable for debugging purposes.

joguSD commented 8 years ago

Sounds good, I'll add logging in the case that we drop the key.

kyleknap commented 8 years ago

Thanks. :ship: