From 7eade2cb521c343781cdf63822b13b0a47ebeed1 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 28 Apr 2021 16:38:14 -0400 Subject: [PATCH] add more diagnostics context There were some remaining calls to provider where configuration could be added to diagnostics, where warnings would not get config annotations, or the diagnostics were skipped entirely. --- terraform/node_provider.go | 73 +++++++++----------- terraform/node_provider_test.go | 31 ++++++++- terraform/node_resource_abstract_instance.go | 13 ++-- 3 files changed, 70 insertions(+), 47 deletions(-) diff --git a/terraform/node_provider.go b/terraform/node_provider.go index 3e70cee08..5b06a25a6 100644 --- a/terraform/node_provider.go +++ b/terraform/node_provider.go @@ -56,13 +56,13 @@ func (n *NodeApplyableProvider) ValidateProvider(ctx EvalContext, provider provi return nil } - resp := provider.GetProviderSchema() - diags = diags.Append(resp.Diagnostics) + schemaResp := provider.GetProviderSchema() + diags = diags.Append(schemaResp.Diagnostics.InConfigBody(configBody, n.Addr.String())) if diags.HasErrors() { return diags } - configSchema := resp.Provider.Block + configSchema := schemaResp.Provider.Block if configSchema == nil { // Should never happen in real code, but often comes up in tests where // mock schemas are being used that tend to be incomplete. @@ -85,7 +85,7 @@ func (n *NodeApplyableProvider) ValidateProvider(ctx EvalContext, provider provi } validateResp := provider.ValidateProviderConfig(req) - diags = diags.Append(validateResp.Diagnostics) + diags = diags.Append(validateResp.Diagnostics.InConfigBody(configBody, n.Addr.String())) return diags } @@ -99,7 +99,7 @@ func (n *NodeApplyableProvider) ConfigureProvider(ctx EvalContext, provider prov configBody := buildProviderConfig(ctx, n.Addr, config) resp := provider.GetProviderSchema() - diags = diags.Append(resp.Diagnostics) + diags = diags.Append(resp.Diagnostics.InConfigBody(configBody, n.Addr.String())) if diags.HasErrors() { return diags } @@ -133,53 +133,44 @@ func (n *NodeApplyableProvider) ConfigureProvider(ctx EvalContext, provider prov // ValidateProviderConfig is only used for validation. We are intentionally // ignoring the PreparedConfig field to maintain existing behavior. - prepareResp := provider.ValidateProviderConfig(req) - if prepareResp.Diagnostics.HasErrors() { - if config == nil { - // If there isn't an explicit "provider" block in the configuration, - // this error message won't be very clear. Add some detail to the - // error message in this case. - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Error, - "Invalid provider configuration", - fmt.Sprintf(providerConfigErr, prepareResp.Diagnostics.Err(), n.Addr.Provider), - )) - return diags - } else { - return diags.Append(prepareResp.Diagnostics) - } + validateResp := provider.ValidateProviderConfig(req) + diags = diags.Append(validateResp.Diagnostics.InConfigBody(configBody, n.Addr.String())) + if diags.HasErrors() && config == nil { + // If there isn't an explicit "provider" block in the configuration, + // this error message won't be very clear. Add some detail to the error + // message in this case. + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Invalid provider configuration", + fmt.Sprintf(providerConfigErr, n.Addr.Provider), + )) + } + + if diags.HasErrors() { + return diags } - diags = diags.Append(prepareResp.Diagnostics) // If the provider returns something different, log a warning to help // indicate to provider developers that the value is not used. - preparedCfg := prepareResp.PreparedConfig + preparedCfg := validateResp.PreparedConfig if preparedCfg != cty.NilVal && !preparedCfg.IsNull() && !preparedCfg.RawEquals(unmarkedConfigVal) { log.Printf("[WARN] ValidateProviderConfig from %q changed the config value, but that value is unused", n.Addr) } configDiags := ctx.ConfigureProvider(n.Addr, unmarkedConfigVal) - if configDiags.HasErrors() { - if config == nil { - // If there isn't an explicit "provider" block in the configuration, - // this error message won't be very clear. Add some detail to the - // error message in this case. - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Error, - "Invalid provider configuration", - fmt.Sprintf(providerConfigErr, configDiags.InConfigBody(configBody, n.Addr.String()).Err(), n.Addr.Provider), - )) - return diags - } else { - return diags.Append(configDiags.InConfigBody(configBody, n.Addr.String())) - } - } diags = diags.Append(configDiags.InConfigBody(configBody, n.Addr.String())) - + if diags.HasErrors() && config == nil { + // If there isn't an explicit "provider" block in the configuration, + // this error message won't be very clear. Add some detail to the error + // message in this case. + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Invalid provider configuration", + fmt.Sprintf(providerConfigErr, n.Addr.Provider), + )) + } return diags } -const providerConfigErr = `%s - -Provider %q requires explicit configuration. Add a provider block to the root module and configure the provider's required arguments as described in the provider documentation. +const providerConfigErr = `Provider %q requires explicit configuration. Add a provider block to the root module and configure the provider's required arguments as described in the provider documentation. ` diff --git a/terraform/node_provider_test.go b/terraform/node_provider_test.go index f52f3b282..4eafe49fa 100644 --- a/terraform/node_provider_test.go +++ b/terraform/node_provider_test.go @@ -316,7 +316,8 @@ func TestNodeApplyableProvider_ConfigProvider(t *testing.T) { provider.ValidateProviderConfigFn = func(req providers.ValidateProviderConfigRequest) (resp providers.ValidateProviderConfigResponse) { region := req.Config.GetAttr("region") if region.IsNull() { - resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("value is not found")) + resp.Diagnostics = resp.Diagnostics.Append( + tfdiags.WholeContainingBody(tfdiags.Error, "value is not found", "you did not supply a required value")) } return } @@ -376,7 +377,7 @@ func TestNodeApplyableProvider_ConfigProvider(t *testing.T) { if !diags.HasErrors() { t.Fatal("missing expected error with invalid config") } - if diags.Err().Error() != "value is not found" { + if !strings.Contains(diags.Err().Error(), "value is not found") { t.Errorf("wrong diagnostic: %s", diags.Err()) } }) @@ -470,3 +471,29 @@ func TestNodeApplyableProvider_ConfigProvider_config_fn_err(t *testing.T) { } }) } + +func TestGetSchemaError(t *testing.T) { + provider := &MockProvider{ + GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{ + Diagnostics: tfdiags.Diagnostics.Append(nil, tfdiags.WholeContainingBody(tfdiags.Error, "oops", "error")), + }, + } + + providerAddr := mustProviderConfig(`provider["terraform.io/some/provider"]`) + ctx := &MockEvalContext{ProviderProvider: provider} + ctx.installSimpleEval() + node := NodeApplyableProvider{ + NodeAbstractProvider: &NodeAbstractProvider{ + Addr: providerAddr, + }, + } + + diags := node.ConfigureProvider(ctx, provider, false) + for _, d := range diags { + desc := d.Description() + if desc.Address != providerAddr.String() { + t.Fatalf("missing provider address from diagnostics: %#v", desc) + } + } + +} diff --git a/terraform/node_resource_abstract_instance.go b/terraform/node_resource_abstract_instance.go index 59227b55a..e27f84e67 100644 --- a/terraform/node_resource_abstract_instance.go +++ b/terraform/node_resource_abstract_instance.go @@ -478,6 +478,10 @@ func (n *NodeAbstractResourceInstance) refresh(ctx EvalContext, state *states.Re } resp := provider.ReadResource(providerReq) + if n.Config != nil { + resp.Diagnostics = resp.Diagnostics.InConfigBody(n.Config.Config, n.Addr.String()) + } + diags = diags.Append(resp.Diagnostics) if diags.HasErrors() { return state, diags @@ -626,8 +630,8 @@ func (n *NodeAbstractResourceInstance) plan( }, ) - if validateResp.Diagnostics.HasErrors() { - diags = diags.Append(validateResp.Diagnostics.InConfigBody(config.Config, n.Addr.String())) + diags = diags.Append(validateResp.Diagnostics.InConfigBody(config.Config, n.Addr.String())) + if diags.HasErrors() { return plan, state, diags } @@ -1195,8 +1199,9 @@ func (n *NodeAbstractResourceInstance) readDataSource(ctx EvalContext, configVal Config: configVal, }, ) - if validateResp.Diagnostics.HasErrors() { - return newVal, validateResp.Diagnostics.InConfigBody(config.Config, n.Addr.String()) + diags = diags.Append(validateResp.Diagnostics.InConfigBody(config.Config, n.Addr.String())) + if diags.HasErrors() { + return newVal, diags } // If we get down here then our configuration is complete and we're read