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

Continue pushing the Path calls out of the Resource and Provider types #24346

Merged
merged 4 commits into from
Mar 11, 2020

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Mar 11, 2020

This starts with some more removal of the Path method from *Resource types, so we can work with them in an unexpanded context.

The majority of this PR is the conversion of the addrs.AbsProviderConfig to use a Module rather than a ModuleInstance. The Abs prefix in AbsProviderConfig is retained for now to reduce the size of the changes, and we can do a wholesale rename later when there is less work in-flight.

Remove the requirement for most *Resource types to be a
GraphNodeModuleInstance, ensuring that they never call ctx.Path while
being evaluated. This gets rid of most of the direct need for the Path
method currently implemented by NodeResourceAbstract, leaving the
provider and schema related calls for a subsequent PR.
Change ModuleInstance to Module in AbsProviderConfig, because providers
need to be handled before module expansion, and should not be used
defined inside an expanded module at all.

Renaming of the addrs type can happen later, when there's less work
in-flight around provider configuration.
@jbardin jbardin requested a review from a team March 11, 2020 01:33
@ghost ghost added the sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK label Mar 11, 2020
}

return fmt.Sprintf("%s.%s[%q]", pc.Module.String(), "provider", pc.Provider.String())
return strings.Join(parts, ".")
Copy link
Contributor

Choose a reason for hiding this comment

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

what a lovely refactor, thank you! That's MUCH nicer to read ❤️

Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

This looks great, thank you for taking care of it! I'll approve now so you can merge as soon as your other PR gets merged.

@jbardin jbardin changed the title Continue pushing the Path calls out of the Provider types Continue pushing the Path calls out of the Resource and Provider types Mar 11, 2020
@jbardin jbardin changed the base branch from jbardin/module-expansion-in-part to master March 11, 2020 15:11
Change all ModuleInstances in provider types to plain Modules
@jbardin jbardin force-pushed the jbardin/module-expansion-another-part branch from 78024d7 to 98cfb51 Compare March 11, 2020 15:22
@jbardin jbardin merged commit 3346456 into master Mar 11, 2020
@jbardin jbardin deleted the jbardin/module-expansion-another-part branch March 11, 2020 18:32
noahmercado pushed a commit to noahmercado/terraform that referenced this pull request Apr 8, 2020
…nsion-another-part

Continue pushing the Path calls out of the Resource and Provider types
@ghost
Copy link

ghost commented Apr 11, 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 Apr 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants