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

resolve provider types when building the config #28414

Merged
merged 1 commit into from
Apr 16, 2021

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Apr 16, 2021

All the information is available to resolve provider types when building
the configuration, but some provider references still had no FQN. This
caused validation to assume a default type, and incorrectly reject valid
module calls with non-default namespaced providers.

Resolve as much provider type information as possible when loading the
config. Only use this internally for now, but this should be useful
outside of the package to avoid re-resolving the providers later on. We
can come back and find where this might be useful elsewhere, but for now
keep the change as small as possible to avoid any changes in behavior.

Fixes #28407

All the information is available to resolve provider types when building
the configuration, but some provider references still had no FQN. This
caused validation to assume a default type, and incorrectly reject valid
module calls with non-default namespaced providers.

Resolve as much provider type information as possible when loading the
config. Only use this internally for now, but this should be useful
outside of the package to avoid re-resolving the providers later on. We
can come back and find where this might be useful elsewhere, but for now
keep the change as small as possible to avoid any changes in behavior.
@jbardin jbardin added the 0.15-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Apr 16, 2021
@jbardin jbardin requested a review from a team April 16, 2021 16:48
@codecov
Copy link

codecov bot commented Apr 16, 2021

Codecov Report

Merging #28414 (d0cc7f1) into main (2cd1619) will increase coverage by 0.01%.
The diff coverage is 91.30%.

Impacted Files Coverage Δ
configs/provider.go 82.52% <ø> (ø)
configs/resource.go 80.95% <ø> (ø)
configs/provider_validation.go 98.29% <71.42%> (-1.71%) ⬇️
configs/config.go 72.86% <100.00%> (+3.56%) ⬆️
configs/config_build.go 82.45% <100.00%> (+0.31%) ⬆️
terraform/node_resource_plan.go 98.05% <0.00%> (+1.94%) ⬆️

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.

LGTM! I have high hopes for that providerType field in the configs.Resource!!

// validation, but once we verify that this can be set in all cases, we can
// export this so providers don't need to be re-resolved.
// This same field is also added to the Provider struct.
providerType addrs.Provider
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember wanting to set this here earlier in the provider source project; I'm glad to see this!

@github-actions
Copy link
Contributor

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 May 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
0.15-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.

TF 0.15 module with multiple openstack providers
2 participants