Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

helper/schema: fix validating nested objects #14784

Merged
merged 1 commit into from
May 24, 2017
Merged

helper/schema: fix validating nested objects #14784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion helper/schema/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1373,7 +1373,7 @@ func (m schemaMap) validateObject(
k string,
schema map[string]*Schema,
c *terraform.ResourceConfig) ([]string, []error) {
raw, _ := c.GetRaw(k)
raw, _ := c.Get(k)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Get will return the Raw value if it's computed, this should probably check for that too. I don't think that validation error helps in the unknown variable case either.

if _, ok := raw.(map[string]interface{}); !ok {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think simply adding ; !ok && !c.IsComputed(k) would suffice here

return nil, []error{fmt.Errorf(
"%s: expected object, got %s",
Expand Down
27 changes: 27 additions & 0 deletions helper/schema/schema_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3947,6 +3947,33 @@ func TestSchemaMap_Validate(t *testing.T) {
Err: false,
},

"Good sub-resource, interpolated value": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're already in here, I think a test with a computed value (that fails before the IsComputed check) would be useful too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit unclear to me what IsComputed is actually checking for. Can you provide me with an example? Is this something like, foo.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I got it, but I'm unable to put together a failing test.

The best case I could come up with was:

		"Bad sub-resource, interpolated value": {
			Schema: map[string]*Schema{
				"ingress": &Schema{
					Type:     TypeList,
					Optional: true,
					Elem: &Resource{
						Schema: map[string]*Schema{
							"from": &Schema{
								Type:     TypeInt,
								Required: true,
							},
						},
					},
				},
			},

			Config: map[string]interface{}{
				"ingress": []interface{}{
					`${map("from", var.port)}`,
				},
			},

			Vars: map[string]string{
				"var.port": config.UnknownVariableValue,
			},

			Err: true,
		},

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IsComputed basically checks if the value is known during validation, i.e. it's in the config and doesn't contain any UnknownValues. If you look at the source to Get, it's actually going to call IsComputed there too, and otherwise return GetRaw.

I think the PR is still good on its own, so I can merge this and I'll send a follow up PR with the additional changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IsComputed basically checks if the value is known during validation, i.e. it's in the config and doesn't contain any UnknownValues.

Yeah, I got that far. :) I was moreso struggling to construct a test case that proves that additional check is required.

I'll send a follow up PR with the additional changes.

Mind cc'ing me on this? I'd like to see what I'm missing here for the future.

Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, started typing before I saw your update...
That looks right, and I swear I had it fail when I tested it. Let me see what's going on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, it's passing because Err: true,. 😉

You'll just need to change Required to Optional here to pass the test, since there there is no real value here.

Schema: map[string]*Schema{
"ingress": &Schema{
Type: TypeList,
Optional: true,
Elem: &Resource{
Schema: map[string]*Schema{
"from": &Schema{
Type: TypeInt,
Required: true,
},
},
},
},
},

Config: map[string]interface{}{
"ingress": []interface{}{
`${map("from", "80")}`,
},
},

Vars: map[string]string{},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was needed to trigger interpolation. Otherwise it's skipped and map() is never resolved since tc.Vars == nil.


Err: false,
},

"Invalid/unknown field": {
Schema: map[string]*Schema{
"availability_zone": &Schema{
Expand Down