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

import blocks are evaluated in the root module #33897

Merged
merged 3 commits into from
Sep 19, 2023
Merged

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Sep 14, 2023

Even though import block evaluation is done in conjunction with their target resource block, their evaluation scope must always be the containing module of the import block. Since import blocks currently are only valid in the root module, that means the context must always always be the root module.

The references for the import evaluation must also be scoped to the root module where the block was written. In order to accomplish that without adding a new import node type to the graph, we instead can opt to add a new method (and associated GraphNodeImportReferencer interface) which separates the import references from the config references. The ReferenceTransformer can then use the correct root module path to lookup the referenced subjects when they come from ImportReferences as opposed to References.

Fixes #33896

Even though import block evaluation is done in conjunction with their
target resource block, their evaluation scope must always be the
containing module of the import block. Since import blocks currently are
only valid in the root module, that means the context must always always
be the root module.
@jbardin jbardin added the 1.6-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Sep 14, 2023
@jbardin jbardin added this to the v1.6.0 milestone Sep 14, 2023
@jbardin jbardin requested a review from kmoe September 14, 2023 16:07
kmoe
kmoe previously approved these changes Sep 14, 2023
@jbardin
Copy link
Member Author

jbardin commented Sep 14, 2023

It looks like References didn't take this split-scope into account either, so I'm not sure fixing the evaluation alone is a good idea.

When an import block is being evaluated by a resource within a module,
the references for the import evaluation must be scoped to the root
module where the block was written. In order to accomplish that without
adding a new import node type to the graph, we instead can opt to add a
new method which separates the import references from the config
references. The ReferenceTransformer can then use the correct root
module path to lookup the referenced subjects when they come from
ImportReferences as opposed to References.
@jbardin
Copy link
Member Author

jbardin commented Sep 14, 2023

I decided to handle the reference scope directly in this PR since it wasn't too daunting of a change.

Copy link
Member

@kmoe kmoe left a comment

Choose a reason for hiding this comment

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

I'm curious as to why the first commit only failed the race tests. I also couldn't replicate that failure locally.

@jbardin
Copy link
Member Author

jbardin commented Sep 19, 2023

It wasn't specifically a race condition that failed, it just happened to fail an import test during that run. Without the correctly scoped references, there wasn't anything preventing the import block form being evaluated before any of its dependencies.

@jbardin jbardin merged commit b2f36c4 into main Sep 19, 2023
4 checks passed
@jbardin jbardin deleted the jbardin/import-id-eval branch September 19, 2023 12:56
@github-actions
Copy link

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@kmoe
Copy link
Member

kmoe commented Sep 19, 2023

Shouldn't merging this have closed #33896 automatically?

@jbardin
Copy link
Member Author

jbardin commented Sep 19, 2023

GH is experiencing some delays processing PR related events. Notification are coming in late too

Copy link

github-actions bot commented Dec 9, 2023

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 Dec 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.6-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.

ID Interpolation not working
2 participants