Parameter guessing #97
Conversation
akdor1154
force-pushed
the
parameterGuessing
branch
2 times, most recently
from
d8438d1
to
60d63f4
Compare
|
@akdor1154 Can you resolve the conflict? I don't have permission to write back to your branch. Ta |
akdor1154
force-pushed
the
parameterGuessing
branch
from
60d63f4
to
9764d85
Compare
|
whoops sorry, I thought that was on by default! |
README.md
Outdated
| `--pseudo <psuedo param values>`: Provide a list of comma-separated key=value pairs of CloudFormation pseudo-parameters to use when validating your template. e.g. | ||
| - `--pseudo AWS::Region=ap-southeast-2` | ||
|
|
||
| `--no-guess-parameters`: Disable the guessing of parameters if they don't have a Default. If you don't provide them on the CLI in this situation, a critical error will be raised instead of the parameter value being mocked. |
There was a problem hiding this comment.
Can we have the default behaviour specified if these flags are omitted?
Leaving as
undefinedpreserves the default loose behaviour, where parameters are guessed as needed without causing an error.
src/test/indexTest.ts
Outdated
| }).timeout(5000); | ||
|
|
||
| it('guess-parameters should allow opting in to parameter mocking', (done) => { | ||
| exec('node lib/index.js validate testData/valid/yaml/no-guess-parameters.yaml --guess-parameters Param1', function(error, stdout, stderr) { |
There was a problem hiding this comment.
Can we have a test for multiple parameters specified for guess-parameters?
--guess parameters Param1, Param 2
src/validator.ts
Outdated
| const logLevel = (okToGuess) | ||
| ? 'info' | ||
| : 'crit'; | ||
| addError(logLevel, 'Guessing parameter value', ['Parameters', parameterName], 'Parameters') |
There was a problem hiding this comment.
This could be misleading; if I specify --no-guess-parameters I would be confused to see a critical error saying Guessing parameter value (It shouldn't be guessing my parameter values!). Maybe something different for this use case would be better. Value for parameter not provided?
There was a problem hiding this comment.
Yeah that would be sensible. Does it still make sense to guess in this case (it kind of has to continue validation...)?
There was a problem hiding this comment.
I've also removed the output completely if okToGuess is true (so behaviour is identical to current)
There was a problem hiding this comment.
Good shout, maybe something like:
If parameter name IN --guess--parameters OR guessAll => "Guessing parameter value" (INFO)
If --no-guess-parameters => "Value for parameter not provided, continuing validation with empty input"
Then we are enforcing what the user expects by using the CLI option.
There was a problem hiding this comment.
Can I leave off the INFO message completely? It currently works like that and would lead to lots of noise in the output. (I think I was mistaken in adding it.)
|
Decided to change |
|
@martysweet were you waiting on me to fix anything up for this? I'm mildly against having an info message when a parameter is guessed in normal mode, simply because there isn't one now and I wouldn't want to change default behaviour (also it'd lead to some console spam), but I can add it back if you think it would be beneficial. |
|
@akdor1154 Agreed. Could you add the following to the CHANGELOG under "Unreleased". (Or something which describes it better) Will release a new version later today! |
akdor1154
force-pushed
the
parameterGuessing
branch
from
177918b
to
e9cc8e9
Compare
|
should be good, thanks! |
Implement strict control over how parameter values are guessed.
By default, parameters guessing is unchanged from current behaviour. The parameter will be guessed by looking at the param Default, AllowedValues, and Type properties in that order.
If the
guessParametersoption is set (to a list of parameter names), only parameters which are in that list will be guessed without reporting an error. Any that are absent and that still need to be guessed will cause a critical error to be raised.From the CLI side this is controlled by the
guess-parameters/no-guess-parametersflags.From the API side this is controlled by just setting guessParameters in the options - an empty list can be used to disable guessing everything.