Skip to content

command/show (-json): fix panic if a moduleCall has a nil config#21569

Merged
mildwonkey merged 2 commits intomasterfrom
mildwonkey/b-json-config-crash
Jun 3, 2019
Merged

command/show (-json): fix panic if a moduleCall has a nil config#21569
mildwonkey merged 2 commits intomasterfrom
mildwonkey/b-json-config-crash

Conversation

@mildwonkey
Copy link
Copy Markdown
Contributor

In the unlikely event that a moduleCall has a nil config - for example,
if a nested module call includes a variable with a typo in an
attribute - continue gracefully.

I am concerned that there are repercussions for sentinel users, so I've opened #21568 to capture the root cause.

Fixes #21543

In the unlikely event that a moduleCall has a nil config - for example,
if a nested module call includes a variable with a typo in an
attribute - continue gracefully.
@mildwonkey mildwonkey requested a review from a team June 3, 2019 15:54
Copy link
Copy Markdown
Contributor

@pselle pselle left a comment

Choose a reason for hiding this comment

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

Makes sense to me! Comment on a comment that might have some room to be clearer to future-us

Comment thread command/jsonconfig/config.go Outdated
}

func marshalModuleCall(c *configs.Config, mc *configs.ModuleCall, schemas *terraform.Schemas) moduleCall {
// It's possible, though unlikely, to have a module call with a nil config
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The issue reference here requires a bit of context loading; I think you can say this directly rather than adding a judgement (the "It's possible, though unlikely" -- what does that comment add?), that a nil config can result from a nested module with a mis-formatted config (if that statement is correct)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The comment is not a judgement so much as a warning meant to emphasize that this is not a normal case, and indeed we may end up fixing the root cause. It's loosely following pattern I see in similar "this doesn't happen often" terraform comments. I will re-word this.

… not happen"

If #21543 is
fixed, we can remove this check.
@mildwonkey mildwonkey merged commit e9e718b into master Jun 3, 2019
@mildwonkey mildwonkey deleted the mildwonkey/b-json-config-crash branch June 3, 2019 18:04
jbardin added a commit that referenced this pull request Jul 17, 2019
One of the show json command tests expected no error when presented with
an invalid configuration in a nested module. Modify the test created in
PR #21569 so that it can still verify there is no panic, but now expect
an error from init.
jbardin added a commit that referenced this pull request Jul 17, 2019
One of the show json command tests expected no error when presented with
an invalid configuration in a nested module. Modify the test created in
PR #21569 so that it can still verify there is no panic, but now expect
an error from init.
@ghost
Copy link
Copy Markdown

ghost commented Jul 25, 2019

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 ghost locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

2 participants