From e574f33fa0ffa72f8cbe145ad860eb16d04fc10a Mon Sep 17 00:00:00 2001 From: Matt Ellis Date: Mon, 20 May 2019 13:56:27 -0700 Subject: [PATCH] Include URN as an argument in DiffConfig/CheckConfig For provider plugins, the gRPC interfaces expect that a URN would be included as part of the DiffConfig/CheckConfig request, which means we need to flow this value into our Provider interface. This change does that. --- pkg/engine/lifecycle_test.go | 8 +++--- pkg/resource/deploy/builtins.go | 5 ++-- pkg/resource/deploy/deploytest/provider.go | 15 +++++------ pkg/resource/deploy/providers/registry.go | 12 +++++---- .../deploy/providers/registry_test.go | 25 +++++++++++-------- pkg/resource/plugin/provider.go | 4 +-- pkg/resource/plugin/provider_plugin.go | 5 ++-- 7 files changed, 40 insertions(+), 34 deletions(-) diff --git a/pkg/engine/lifecycle_test.go b/pkg/engine/lifecycle_test.go index 0710cbd29..598dad41c 100644 --- a/pkg/engine/lifecycle_test.go +++ b/pkg/engine/lifecycle_test.go @@ -715,7 +715,7 @@ func TestSingleResourceDefaultProviderReplace(t *testing.T) { loaders := []*deploytest.ProviderLoader{ deploytest.NewProviderLoader("pkgA", semver.MustParse("1.0.0"), func() (plugin.Provider, error) { return &deploytest.Provider{ - DiffConfigF: func(olds, news resource.PropertyMap) (plugin.DiffResult, error) { + DiffConfigF: func(urn resource.URN, olds, news resource.PropertyMap) (plugin.DiffResult, error) { // Always require replacement. keys := []resource.PropertyKey{} for k := range news { @@ -793,7 +793,7 @@ func TestSingleResourceExplicitProviderReplace(t *testing.T) { loaders := []*deploytest.ProviderLoader{ deploytest.NewProviderLoader("pkgA", semver.MustParse("1.0.0"), func() (plugin.Provider, error) { return &deploytest.Provider{ - DiffConfigF: func(olds, news resource.PropertyMap) (plugin.DiffResult, error) { + DiffConfigF: func(urn resource.URN, olds, news resource.PropertyMap) (plugin.DiffResult, error) { // Always require replacement. keys := []resource.PropertyKey{} for k := range news { @@ -882,7 +882,7 @@ func TestSingleResourceExplicitProviderDeleteBeforeReplace(t *testing.T) { loaders := []*deploytest.ProviderLoader{ deploytest.NewProviderLoader("pkgA", semver.MustParse("1.0.0"), func() (plugin.Provider, error) { return &deploytest.Provider{ - DiffConfigF: func(olds, news resource.PropertyMap) (plugin.DiffResult, error) { + DiffConfigF: func(urn resource.URN, olds, news resource.PropertyMap) (plugin.DiffResult, error) { // Always require replacement. keys := []resource.PropertyKey{} for k := range news { @@ -2598,7 +2598,7 @@ func TestDeleteBeforeReplace(t *testing.T) { loaders := []*deploytest.ProviderLoader{ deploytest.NewProviderLoader("pkgA", semver.MustParse("1.0.0"), func() (plugin.Provider, error) { return &deploytest.Provider{ - DiffConfigF: func(olds, news resource.PropertyMap) (plugin.DiffResult, error) { + DiffConfigF: func(urn resource.URN, olds, news resource.PropertyMap) (plugin.DiffResult, error) { if !olds["A"].DeepEquals(news["A"]) { return plugin.DiffResult{ ReplaceKeys: []resource.PropertyKey{"A"}, diff --git a/pkg/resource/deploy/builtins.go b/pkg/resource/deploy/builtins.go index cca253b82..ba3b2dd52 100644 --- a/pkg/resource/deploy/builtins.go +++ b/pkg/resource/deploy/builtins.go @@ -38,15 +38,14 @@ func (p *builtinProvider) Pkg() tokens.Package { } // CheckConfig validates the configuration for this resource provider. -func (p *builtinProvider) CheckConfig(olds, +func (p *builtinProvider) CheckConfig(urn resource.URN, olds, news resource.PropertyMap) (resource.PropertyMap, []plugin.CheckFailure, error) { return nil, nil, nil } // DiffConfig checks what impacts a hypothetical change to this provider's configuration will have on the provider. -func (p *builtinProvider) DiffConfig(olds, news resource.PropertyMap) (plugin.DiffResult, error) { - +func (p *builtinProvider) DiffConfig(urn resource.URN, olds, news resource.PropertyMap) (plugin.DiffResult, error) { return plugin.DiffResult{Changes: plugin.DiffNone}, nil } diff --git a/pkg/resource/deploy/deploytest/provider.go b/pkg/resource/deploy/deploytest/provider.go index 00d397d15..afcc38727 100644 --- a/pkg/resource/deploy/deploytest/provider.go +++ b/pkg/resource/deploy/deploytest/provider.go @@ -32,9 +32,10 @@ type Provider struct { configured bool - CheckConfigF func(olds, news resource.PropertyMap) (resource.PropertyMap, []plugin.CheckFailure, error) - DiffConfigF func(olds, news resource.PropertyMap) (plugin.DiffResult, error) - ConfigureF func(news resource.PropertyMap) error + CheckConfigF func(urn resource.URN, olds, + news resource.PropertyMap) (resource.PropertyMap, []plugin.CheckFailure, error) + DiffConfigF func(urn resource.URN, olds, news resource.PropertyMap) (plugin.DiffResult, error) + ConfigureF func(news resource.PropertyMap) error CheckF func(urn resource.URN, olds, news resource.PropertyMap) (resource.PropertyMap, []plugin.CheckFailure, error) @@ -75,18 +76,18 @@ func (prov *Provider) GetPluginInfo() (workspace.PluginInfo, error) { }, nil } -func (prov *Provider) CheckConfig(olds, +func (prov *Provider) CheckConfig(urn resource.URN, olds, news resource.PropertyMap) (resource.PropertyMap, []plugin.CheckFailure, error) { if prov.CheckConfigF == nil { return news, nil, nil } - return prov.CheckConfigF(olds, news) + return prov.CheckConfigF(urn, olds, news) } -func (prov *Provider) DiffConfig(olds, news resource.PropertyMap) (plugin.DiffResult, error) { +func (prov *Provider) DiffConfig(urn resource.URN, olds, news resource.PropertyMap) (plugin.DiffResult, error) { if prov.DiffConfigF == nil { return plugin.DiffResult{}, nil } - return prov.DiffConfigF(olds, news) + return prov.DiffConfigF(urn, olds, news) } func (prov *Provider) Configure(inputs resource.PropertyMap) error { contract.Assert(!prov.configured) diff --git a/pkg/resource/deploy/providers/registry.go b/pkg/resource/deploy/providers/registry.go index 56ce56033..d2c722986 100644 --- a/pkg/resource/deploy/providers/registry.go +++ b/pkg/resource/deploy/providers/registry.go @@ -185,13 +185,15 @@ func (r *Registry) label() string { } // CheckConfig validates the configuration for this resource provider. -func (r *Registry) CheckConfig(olds, news resource.PropertyMap) (resource.PropertyMap, []plugin.CheckFailure, error) { +func (r *Registry) CheckConfig(urn resource.URN, olds, + news resource.PropertyMap) (resource.PropertyMap, []plugin.CheckFailure, error) { + contract.Fail() return nil, nil, errors.New("the provider registry is not configurable") } // DiffConfig checks what impacts a hypothetical change to this provider's configuration will have on the provider. -func (r *Registry) DiffConfig(olds, news resource.PropertyMap) (plugin.DiffResult, error) { +func (r *Registry) DiffConfig(urn resource.URN, olds, news resource.PropertyMap) (plugin.DiffResult, error) { contract.Fail() return plugin.DiffResult{}, errors.New("the provider registry is not configurable") } @@ -231,7 +233,7 @@ func (r *Registry) Check(urn resource.URN, olds, news resource.PropertyMap, } // Check the provider's config. If the check fails, unload the provider. - inputs, failures, err := provider.CheckConfig(olds, news) + inputs, failures, err := provider.CheckConfig(urn, olds, news) if len(failures) != 0 || err != nil { closeErr := r.host.CloseProvider(provider) contract.IgnoreError(closeErr) @@ -274,7 +276,7 @@ func (r *Registry) Diff(urn resource.URN, id resource.ID, olds, news resource.Pr provider, ok = r.GetProvider(mustNewReference(urn, id)) contract.Assertf(ok, "Provider must have been registered by NewRegistry for DBR Diff (%v::%v)", urn, id) - diff, err := provider.DiffConfig(olds, news) + diff, err := provider.DiffConfig(urn, olds, news) if err != nil { return plugin.DiffResult{Changes: plugin.DiffUnknown}, err } @@ -282,7 +284,7 @@ func (r *Registry) Diff(urn resource.URN, id resource.ID, olds, news resource.Pr } // Diff the properties. - diff, err := provider.DiffConfig(olds, news) + diff, err := provider.DiffConfig(urn, olds, news) if err != nil { return plugin.DiffResult{Changes: plugin.DiffUnknown}, err } diff --git a/pkg/resource/deploy/providers/registry_test.go b/pkg/resource/deploy/providers/registry_test.go index 7e003e6fa..d400ba317 100644 --- a/pkg/resource/deploy/providers/registry_test.go +++ b/pkg/resource/deploy/providers/registry_test.go @@ -77,9 +77,10 @@ type testProvider struct { pkg tokens.Package version semver.Version configured bool - checkConfig func(resource.PropertyMap, resource.PropertyMap) (resource.PropertyMap, []plugin.CheckFailure, error) - diffConfig func(resource.PropertyMap, resource.PropertyMap) (plugin.DiffResult, error) - config func(resource.PropertyMap) error + checkConfig func(resource.URN, resource.PropertyMap, + resource.PropertyMap) (resource.PropertyMap, []plugin.CheckFailure, error) + diffConfig func(resource.URN, resource.PropertyMap, resource.PropertyMap) (plugin.DiffResult, error) + config func(resource.PropertyMap) error } func (prov *testProvider) SignalCancellation() error { @@ -91,12 +92,12 @@ func (prov *testProvider) Close() error { func (prov *testProvider) Pkg() tokens.Package { return prov.pkg } -func (prov *testProvider) CheckConfig(olds, +func (prov *testProvider) CheckConfig(urn resource.URN, olds, news resource.PropertyMap) (resource.PropertyMap, []plugin.CheckFailure, error) { - return prov.checkConfig(olds, news) + return prov.checkConfig(urn, olds, news) } -func (prov *testProvider) DiffConfig(olds, news resource.PropertyMap) (plugin.DiffResult, error) { - return prov.diffConfig(olds, news) +func (prov *testProvider) DiffConfig(urn resource.URN, olds, news resource.PropertyMap) (plugin.DiffResult, error) { + return prov.diffConfig(urn, olds, news) } func (prov *testProvider) Configure(inputs resource.PropertyMap) error { if err := prov.config(inputs); err != nil { @@ -202,10 +203,11 @@ func newSimpleLoader(t *testing.T, pkg, version string, config func(resource.Pro return &testProvider{ pkg: pkg, version: ver, - checkConfig: func(olds, news resource.PropertyMap) (resource.PropertyMap, []plugin.CheckFailure, error) { + checkConfig: func(urn resource.URN, olds, + news resource.PropertyMap) (resource.PropertyMap, []plugin.CheckFailure, error) { return news, nil, nil }, - diffConfig: func(olds, news resource.PropertyMap) (plugin.DiffResult, error) { + diffConfig: func(urn resource.URN, olds, news resource.PropertyMap) (plugin.DiffResult, error) { return plugin.DiffResult{}, nil }, config: config, @@ -512,10 +514,11 @@ func TestCRUDPreview(t *testing.T) { return &testProvider{ pkg: pkg, version: ver, - checkConfig: func(olds, news resource.PropertyMap) (resource.PropertyMap, []plugin.CheckFailure, error) { + checkConfig: func(urn resource.URN, olds, + news resource.PropertyMap) (resource.PropertyMap, []plugin.CheckFailure, error) { return news, nil, nil }, - diffConfig: func(olds, news resource.PropertyMap) (plugin.DiffResult, error) { + diffConfig: func(urn resource.URN, olds, news resource.PropertyMap) (plugin.DiffResult, error) { // Always reuquire replacement. return plugin.DiffResult{ReplaceKeys: []resource.PropertyKey{"id"}}, nil }, diff --git a/pkg/resource/plugin/provider.go b/pkg/resource/plugin/provider.go index 56241203d..abf6c2b02 100644 --- a/pkg/resource/plugin/provider.go +++ b/pkg/resource/plugin/provider.go @@ -39,9 +39,9 @@ type Provider interface { Pkg() tokens.Package // CheckConfig validates the configuration for this resource provider. - CheckConfig(olds, news resource.PropertyMap) (resource.PropertyMap, []CheckFailure, error) + CheckConfig(urn resource.URN, olds, news resource.PropertyMap) (resource.PropertyMap, []CheckFailure, error) // DiffConfig checks what impacts a hypothetical change to this provider's configuration will have on the provider. - DiffConfig(olds, news resource.PropertyMap) (DiffResult, error) + DiffConfig(urn resource.URN, olds, news resource.PropertyMap) (DiffResult, error) // Configure configures the resource provider with "globals" that control its behavior. Configure(inputs resource.PropertyMap) error diff --git a/pkg/resource/plugin/provider_plugin.go b/pkg/resource/plugin/provider_plugin.go index 720f43865..457a3bec9 100644 --- a/pkg/resource/plugin/provider_plugin.go +++ b/pkg/resource/plugin/provider_plugin.go @@ -85,7 +85,8 @@ func (p *provider) label() string { } // CheckConfig validates the configuration for this resource provider. -func (p *provider) CheckConfig(olds, news resource.PropertyMap) (resource.PropertyMap, []CheckFailure, error) { +func (p *provider) CheckConfig(urn resource.URN, olds, + news resource.PropertyMap) (resource.PropertyMap, []CheckFailure, error) { // Ensure that all config values are strings or unknowns. var failures []CheckFailure for k, v := range news { @@ -111,7 +112,7 @@ func (p *provider) CheckConfig(olds, news resource.PropertyMap) (resource.Proper } // DiffConfig checks what impacts a hypothetical change to this provider's configuration will have on the provider. -func (p *provider) DiffConfig(olds, news resource.PropertyMap) (DiffResult, error) { +func (p *provider) DiffConfig(urn resource.URN, olds, news resource.PropertyMap) (DiffResult, error) { // There are two interesting scenarios with the present gRPC interface: // 1. Configuration differences in which all properties are known // 2. Configuration differences in which some new property is unknown.