Use plan graph for importing resources #31283
Conversation
Combine all plan-time graphs into a single graph builder, because _everything is a plan_! Convert the import graph to a plan graph. This should resolve a few edge cases about things not being properly evaluated during import, and takes a step towards being able to _plan_ an import.
jbardin
force-pushed
the
jbardin/plan-import
branch
from
e454ac8 to
77cca0a
Compare
jbardin
force-pushed
the
jbardin/plan-import
branch
from
71bb8ca to
4981942
Compare
Missing required attributes should not prevent importing
| // We also skip refresh for now, since the plan output is written | ||
| // as the new state, and users are not expecting the import process | ||
| // to update any other instances in state. | ||
| skipRefresh: true, |
There was a problem hiding this comment.
Setting skipRefresh to true both here and when creating the PlanGraphBuilder jumped out at me as a place where we might accidentally lose sync. Would it make sense to pass through the b.skipRefresh value here?
There was a problem hiding this comment.
Thanks, that's just an oversight, and should be synced up in some way 👍
| @@ -172,12 +194,7 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer { | |||
| return steps | |||
| } | |||
|
|
|||
| func (b *PlanGraphBuilder) init() { | |||
| // Do nothing if the user requests customizing the fields | |||
| if b.CustomConcrete { | |||
There was a problem hiding this comment.
This field on PlanGraphBuilder now seems to be unused, so I think we could remove it altogether.
|
I'm going to merge this as-is for now so that there's more time to try and shake out any unexpected issues with the already fiddly import process. I will still follow up with the possibility of re-instating the |
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
|
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. |
The first thing done here is that we combine the functionality of all the plan-time graph builders, namely
plan,destroy, andvalidateinto a singleGraphBuilderPlanfor a single entrypoint. This is mostly a continuation of the process started in #31163 to consolidate graph building behaviors.Once we have a single plan graph, we then add the ability to deal with imports, which turns out to be fairly trivial. Rather than using a "transformer" to modify the graph and add individual import nodes, we add the import addresses to the abstract resource nodes, and let them create the correct node during resource expansion. This ensures all instances are accounted for, even if they are not currently planning or refreshing anything.
For now we skip planning and refreshing other instances from config during an import operation. Instance planning will probably need to be added for more complex configurations where the planned values need to be evaluated, but is disabled to try and stay as close the the current behavior as possible for testing (the unit test failures from enabling that are likely all just incomplete test fixtures). Refresh cannot be added until the import process can be fully "planned", since the current result of planning uses the resulting state directly.
Further integrating this into
Context.Plan, and making import act more like a plan option should probably be done in concert with adding import actions to the plan itself, so that we can have the ability to store, compare, and display the imported state rather than blindly overwriting the existing state.Due to the necessity of having existing configuration for more complete planning of imports, the
-allow-missing-configoption is being removed from the import command. This possibility was allowed for in the v1.0 Compatibility Promises under commands that might change.Fixes #30706
Fixes #27934
May apply to #30746 as well, though we don't have a reproduction for that yet.