provisioner/file: refactor the provisioner validation function #15273
Conversation
| numSrc := 0 | ||
| if _, ok := d.GetOk("source"); ok == true { | ||
| numSrc++ | ||
| if d.Get("source").(string) == "" && d.Get("content").(string) == "" { |
There was a problem hiding this comment.
this will not catch the situation when you have both source and content defined
There was a problem hiding this comment.
That is not needed, as that will be catched by the ConflictsWith configs defined in the schema.
There was a problem hiding this comment.
What about tests? This functionality was broken for 2 releases since 0.9.6, and this is core functionality. Would be nice to catch this, so far I can see current tests are not catching the problem described in #15177
There was a problem hiding this comment.
Just added a test to the latest revision of this fix...
|
Working on an improved version of this fix... Will update later this evening. |
|
@apparentlymart could you have a look at this one? I refactored it after the chat we had yesterday and I think the current solution is a very clean one that circumvents all the issues we talked about... |
There was a problem hiding this comment.
Using ResourceConfig instead of ResourceData here seems reasonable, since we know we're only looking at config and so no strong reason to force us through all the compromises the ResourceData API makes.
It'd be good to have some tests for this, to make sure that the right result comes out the other side after going through the various layers here.
|
Of course... Now were happy with the solution, I'll add some tests tomorrow before merging the PR. Thx! |
|
@apparentlymart added a test that fails with the reported error when not using this patch... Guess that wraps up this PR right? |
| @@ -48,6 +48,21 @@ func TestResourceProvider_Validate_good_content(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestResourceProvider_Validate_good_unknown_variable_value(t *testing.T) { | |||
| c := testConfig(t, map[string]interface{}{ | |||
| "content": "74D93920-ED26-11E3-AC10-0800200C9A66", | |||
There was a problem hiding this comment.
I assume this is the "unknown variable value". If possible, it'd be preferable to use one of the global variables that contain this, so that in future when this stops being a magic string we'll be able to track down uses of it to update them.
It's available in a few places, though off the top of my head I'm not sure what's the most convenient to access from here. It's a global variable called UnknownValue or UnknownVariableValue in a few different packages.
It turns out that `d.GetOk` also returns `false` when the user _did_ actually supply a value for it in the config, but the value itself needs to be evaluated before it can be used. So instead of passing a `ResourceData` we now pass a `ResourceConfig` which makes much more sense for doing the validation anyway.
|
@apparentlymart that's actually a very good point... Thx! Updated the test so it uses the constant |
svanharmelen
changed the title
d.GetOk in validation functions
It turns out that `d.GetOk` also returns `false` when the user _did_ actually supply a value for it in the config, but the value itself needs to be evaluated before it can be used. So instead of passing a `ResourceData` we now pass a `ResourceConfig` which makes much more sense for doing the validation anyway.
It turns out that `d.GetOk` also returns `false` when the user _did_ actually supply a value for it in the config, but the value itself needs to be evaluated before it can be used. So instead of passing a `ResourceData` we now pass a `ResourceConfig` which makes much more sense for doing the validation anyway.
|
Hi, Regards, |
|
Yes, indeed. Thanks for the pointer, updated and closed the other issue as well. |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
It turns out that
d.GetOkalso returnsfalsewhen the user did actually supply a value for it in the config, but the value itself needs to be evaluated before it can be used.So instead of passing a
ResourceDatawe now pass aResourceConfigwhich makes much more sense for doing the validation anyway.