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

Expander.ExpandResource cannot expand all modules #24454

Merged
merged 1 commit into from
Mar 25, 2020

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Mar 25, 2020

ExpandResource must only check the specific ModuleInstances in the
requested path, because the resource may not have been registered yet in
all module instances.

ExpandResource must only check the specific ModuleInstances in the
requested path, because the resource may not have been registered yet in
all module instances.
@jbardin jbardin requested a review from a team March 25, 2020 15:59
Copy link
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.

Just a question -- maybe needs a comment? but generally lgtm

}
moduleInstanceAddr := make(addrs.ModuleInstance, 0, 4)
ret := e.exps.resourceInstances(resourceAddr.Module, resourceAddr.Resource, moduleInstanceAddr)
sort.SliceStable(ret, func(i, j int) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these getting sorted?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just for consistency, so that things like output and test fixtures are stable.
It also matched the behavior of the original method above, now called ExpandModuleResource.

@jbardin jbardin merged commit c7e2bac into master Mar 25, 2020
@jbardin jbardin deleted the jbardin/expand-resource branch March 25, 2020 19:34
@ghost
Copy link

ghost commented Apr 25, 2020

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 Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants