Skip to content

Conversation

@joguSD
Copy link
Contributor

@joguSD joguSD commented Aug 25, 2016

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
Copy link

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
Copy link
Contributor Author

joguSD commented Aug 26, 2016

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
Copy link

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
Copy link
Contributor Author

joguSD commented Aug 26, 2016

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

@kyleknap
Copy link

Thanks. 🚢

@joguSD joguSD merged commit 6f43f8e into awslabs:wizard-feature Aug 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants