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

terraform: Further generalization of the EvalContext scope handling #34614

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

apparentlymart
Copy link
Contributor

This is a continuation of some restructuring started in #34571.

In those earlier commits my goal was to minimize breaking changes to the EvalContext API to churn existing code as little as possible.

However, the way I achieved that was to handle fully-expanded modules and partial-expanded modules as two completely separate codepaths, which means that all other code must statically decide which of the two cases it's dealing with even though there's not really any reason the EvalContext implementation couldn't support that being decided dynamically.

For the "deferred actions" work, in a future commit we'll need to introduce a new graph node type representing partial-expanded resources which must decide dynamically whether it's in a fully-expanded module scope or a partial-expanded module scope, because the treatment is quite different for a fully-expanded module containing a resource with unknown expansion vs. a resource that's inside a module that hasn't been expanded itself.

This is therefore the logical conclusion of the previous work, making "eval context scope" the primary way that we talk about where an eval context is doing its work, with a helper function for the common case where that's a fully-expanded ModuleInstance.

In particular, this includes a third variation of the family of interfaces that have method Path indicating the scope that the node should be evaluated in. graphNodeEvalContextScope.Path may return any non-nil evalContextScope value chosen dynamically at runtime. The existing interfaces that statically return addrs.ModuleInstance and addrs.PartialExpandedModule respectively remain as statically-typed alternatives for the common case, since the need to decide dynamically is (for now) limited only to nodes representing addrs.PartialExpandedResource addresses.

This has unfortunately now churned a little more code than I originally wanted to, but the helper functions and the retaining of the statically-typed node-scope-discovery interfaces has kept it as small as possible.

(This also continues our effort to gradually make the implementation details of package terraform be unexported, at the expense of a little inconsistency in the short term where these new fields/types are mixed in with exported fields/types. Hopefully we'll continue on this quest over time and eventually reach things being consistently unexported.)


The first (and, for now, only) implementation of graphNodeEvalContextScope will follow in a future PR, adding a new graph node representing what the addrs.PartialExpandedResource address type represents: either an unknown resource instance address inside a known module instance address, or an unknown resource instance address inside an unknown module instance address.

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.

This is hard for me to fully comprehend without a concrete implementation of graphNodeEvalContextScope, but it looks like a good direction as far as I can understand it!

This is a continuation of some restructuring started in earlier commits:
    641837d
    42f012b
    079021d

In those earlier commits my goal was to minimize breaking changes to the
EvalContext API to churn existing code as little as possible.

However, the way I achieved that was to handle fully-expanded modules and
partial-expanded modules as two completely separate codepaths, which means
that all other code must statically decide which of the two cases it's
dealing with even though there's not really any reason the EvalContext
implementation couldn't support that being decided dynamically.

For the "deferred actions" work, in a future commit we'll need to
introduce a new graph node type representing partial-expanded resources
which must decide dynamically whether it's in a fully-expanded module
scope or a partial-expanded module scope, because the treatment is quite
different for a fully-expanded module containing a resource with unknown
expansion vs. a resource that's inside a module that hasn't been expanded
itself.

This is therefore the logical conclusion of the previous work, making
"eval context scope" the primary way that we talk about where an eval
context is doing its work, with a helper function for the common case
where that's a fully-expanded ModuleInstance.

In particular, this includes a third variation of the family of interfaces
that have method Path indicating the scope that the node should be
evaluated in, which is allowed to return any non-nil evalContextScope
chosen dynamically at runtime. The existing interfaces that statically
return addrs.ModuleInstance and addrs.PartialExpandedModule remain as
statically-typed alternatives for the common case, since the need to
decide dynamically is (for now) limited only to nodes representing
addrs.PartialExpandedResource addresses.

This has unfortunately now churned a little more code than I originally
wanted to, but the helper functions and the retaining of the
statically-typed node-scope-discovery interfaces has kept it as small as
possible.

(This also continues our effort to gradually make the implementation
details of package terraform be unexported, at the expense of a little
inconsistency in the short term where these new fields/types are mixed
in with exported fields/types. Hopefully we'll continue on this quest
over time and eventually reach things being consistently unexported.)
@apparentlymart apparentlymart force-pushed the f-evalcontext-scope-dynamic branch from abaa029 to 0591e40 Compare February 6, 2024 18:30
@apparentlymart apparentlymart merged commit 34ccf28 into main Feb 6, 2024
6 checks passed
@apparentlymart apparentlymart deleted the f-evalcontext-scope-dynamic branch February 6, 2024 18:35
Copy link
Contributor

github-actions bot commented Feb 6, 2024

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

Copy link
Contributor

github-actions bot commented Mar 8, 2024

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

Successfully merging this pull request may close these issues.

2 participants