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.

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

ensure we record diagnostics from nested modules #22098

Merged
merged 2 commits into from
Jul 17, 2019

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Jul 16, 2019

When loading nested modules, the child module diagnostics were dropped
in the recursive function. This mean that the config from the submodules
wasn't fully loaded, even though no errors were reported to the user.

This caused further problems if the plan was stored in a plan file, which
means only the partial configuration was stored for the subsequent apply
operation, which would result in unexplained "Resource node has no
configuration attached" errors later on.

Also due to the child module diagnostics being lost, any newly added
nested modules would be silently ignored until init was run again
manually.

Fixes #21757
Fixes #21624
Fixes #21515
Fixes #21771

When loading nested modules, the child module diagnostics were dropped
in the recursive function. This mean that the config from the submodules
wasn't fully loaded, even though no errors were reported to the user.

This caused further problems if the plan was stored in a plan file, when
means only the partial configuration was stored for the subsequent apply
operation, which would result in unexplained "Resource node has no
configuration attached" errors later on.

Also due to the child module diagnostics being lost, any newly added
nested modules would be silently ignored until `init` was run again
manually.
@jbardin jbardin requested a review from a team July 16, 2019 23:08
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

This seems to have identified a test that was depending on the error being ignored and is now failing, but aside from that this makes sense to me!

@jbardin
Copy link
Member Author

jbardin commented Jul 17, 2019

I was expecting to find a test fixture error, but not one introduced on purpose! Fixed up the test so it still verifies there's no panic, but an error should be expected from an invalid configuration.

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 jbardin force-pushed the jbardin/nested-module-diags branch from 5d2fbae to e4640a4 Compare July 17, 2019 01:30
@jbardin jbardin merged commit d770a16 into master Jul 17, 2019
@jbardin jbardin deleted the jbardin/nested-module-diags branch July 17, 2019 01:48
@ghost
Copy link

ghost commented Aug 16, 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 Aug 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants