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

Use plan graph for importing resources #31283

Merged
merged 7 commits into from Jun 23, 2022
Merged

Use plan graph for importing resources #31283

merged 7 commits into from Jun 23, 2022

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Jun 20, 2022

The first thing done here is that we combine the functionality of all the plan-time graph builders, namely plan, destroy, and validate into a single GraphBuilderPlan for 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-config option 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.

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 jbardin requested a review from a team June 20, 2022 19:57
Missing required attributes should not prevent importing
Copy link
Member

@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.

Neat! I smoke tested this briefly locally and all seemed well.

A couple of minor thoughts inline from reading the diff.

// 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,
Copy link
Member

Choose a reason for hiding this comment

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

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?

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, 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 {
Copy link
Member

Choose a reason for hiding this comment

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

This field on PlanGraphBuilder now seems to be unused, so I think we could remove it altogether.

@jbardin
Copy link
Member Author

jbardin commented Jun 23, 2022

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 -allow-missing-config for broader compatibility with existing workflows.

@jbardin jbardin merged commit 77e6b62 into main Jun 23, 2022
@jbardin jbardin deleted the jbardin/plan-import branch June 23, 2022 17:27
@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.

@github-actions
Copy link

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 Jul 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants