-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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.
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
fix(misconf): properly expand dynamic blocks #7612
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: nikpivkin <[email protected]>
Signed-off-by: nikpivkin <[email protected]>
@@ -567,7 +567,7 @@ resource "something" "else" { | |||
for_each = toset(["true"]) | |||
|
|||
content { | |||
ok = each.value | |||
ok = blah.value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reviewers: this test should not have run successfully because dynamic blocks do not create an each
object.
expected: []any{"ssh", "http", "https"}, | ||
}, | ||
{ | ||
name: "map key", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we rename the case accordingly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 1a3282c
} | ||
} | ||
}`, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add an explicit expected
block with the nil value or a comment to explain it so we don't think it's been missed by mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 1a3282c
|
||
realBlockType := b.TypeLabel() | ||
if realBlockType == "" { | ||
return nil, errors.New("dynamic block must have 1 label") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we seem to have been using fmt.Errorf
elsewhere, should we stick to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fmt.Errorf
is used to format the string
return expanded, errors.Join(errs...) | ||
} | ||
|
||
func (b *Block) validateForEach() (cty.Value, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add test cases for the error paths in this func?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test covers those cases
Signed-off-by: nikpivkin <[email protected]>
Description
This PR separates the logic for expanding dynamic blocks from expanding the
for-each
meta argument.Related issues
Checklist