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

Ignore unexpanded paths when validating move statements. #30189

Merged
merged 2 commits into from
Dec 17, 2021

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Dec 16, 2021

After each plan, the entire set of known move blocks must be validated. Sometimes the individual addresses may lie outside of paths which are expanded during the plan, either through the use of -target, or later changes to the configuration that make the moved obsolete.

Since instances.Set is only used after all instances have been processed, it should only handle known instances and not panic when given an address that traverses an unexpanded module. This will allow for validation of moved block which no longer apply to the current set of address to skip over addresses which cannot be expanded.

Fixes #30196
Fixes #30184

@jbardin jbardin changed the title validate moves Ignore unexpanded paths when validating move statements. Dec 17, 2021
@jbardin jbardin requested review from a team and removed request for apparentlymart December 17, 2021 18:06
@jbardin jbardin added the 1.1-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Dec 17, 2021
@jbardin jbardin self-assigned this Dec 17, 2021
@jbardin jbardin force-pushed the jbardin/validate-moves branch 2 times, most recently from fb73d90 to e860428 Compare December 17, 2021 18:30
instances.Set is only used after all instances have been processes, so
it should therefor only handle known instances and not panic when given
an address that traverses an unexpanded module.
@jbardin jbardin marked this pull request as ready for review December 17, 2021 18:45
@apparentlymart
Copy link
Contributor

I reviewed the source code of ValidateMoves to confirm that it only contains rules of the form "if given address exists in the set, error" and no rules of the form "if given address doesn't exist in the set, error", which is what I was hoping to confirm to give confidence that generating a partial set would be "safe" in the sense of potentially not catching things it would normally catch, rather than catching extra things it ought not to catch.

Copy link
Contributor

@lafentres lafentres left a comment

Choose a reason for hiding this comment

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

this makes sense and i can confirm it fixes the error for the reproduction config i was able to create 👍🏼

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.1-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TF 1.1.x no expansion has been registered for module. Invalid memory address or nil pointer dereference
3 participants