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

Generalize target addresses before expansion #25760

Merged
merged 2 commits into from
Aug 12, 2020
Merged

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Aug 6, 2020

Before expansion happens, we only have expansion resource nodes that
know their ConfigResource address. In order to properly compare these to
targets within a module instance, we need to generalize the target to
also be a ConfigResource.

We can also remove the IgnoreIndices field from the transformer, since
we have addresses that are properly scoped and can compare them in the
correct context.

Fixes #25748

@jbardin jbardin requested a review from a team August 6, 2020 14:16
@codecov
Copy link

codecov bot commented Aug 6, 2020

Codecov Report

Merging #25760 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
terraform/graph_builder_plan.go 100.00% <ø> (ø)
terraform/graph_builder_refresh.go 100.00% <ø> (ø)
terraform/transform_targets.go 96.96% <100.00%> (+0.14%) ⬆️
terraform/node_resource_plan.go 93.44% <0.00%> (+1.63%) ⬆️
internal/providercache/dir.go 75.86% <0.00%> (+5.17%) ⬆️

@jbardin jbardin added this to the v0.13.1 milestone Aug 6, 2020
Copy link
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

The resource change makes sense to me. I have a question about targeting a module instance, but it's an aside and needn't block merge ✅

case addrs.AbsResourceInstance:
targetAddr = instance.ContainingResource().Config()
case addrs.ModuleInstance:
targetAddr = instance.Module()
Copy link
Contributor

Choose a reason for hiding this comment

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

I was curious about why the module case was removed here, and tried to check if targeting a module instance would work. It doesn't seem to (unless my test is broken), and I couldn't find a way to make it work, but I'm very unfamiliar with this code so that's not surprising.

Should terraform apply -target=module.mod[1] work? Does it already and I'm just mistaken?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @alisdair! It appears I was thinking about this block in terms of only resource addresses (probably because of the case statement), and not the module itself. Let me add some tests in here too, since that was obviously not covered.

Copy link
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

✅ 2️⃣

Before expansion happens, we only have expansion resource nodes that
know their ConfigResource address. In order to properly compare these to
targets within a module instance, we need to generalize the target to
also be a ConfigResource.

We can also remove the IgnoreIndices field from the transformer, since
we have addresses that are properly scoped and can compare them in the
correct context.
When working with a ConfigResource, the generalization of a
ModuleInstance to a Module was inadvertently dropped, and there was to
test coverage for that type of target.

Ensure we can target a specific module instance alone.
@jbardin jbardin force-pushed the jbardin/target-resource branch from cc766aa to b9e076e Compare August 12, 2020 14:23
@jbardin jbardin merged commit aec0059 into master Aug 12, 2020
@jbardin jbardin deleted the jbardin/target-resource branch August 12, 2020 14:28
@ghost
Copy link

ghost commented Sep 12, 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 Sep 12, 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.

Cannot target a resource inside a module with for_each
2 participants