From 72a6ed320c5982b1852c4d953feb912477b9fad7 Mon Sep 17 00:00:00 2001 From: Pat Gavlin Date: Tue, 23 Oct 2018 10:23:28 -0700 Subject: [PATCH] Do not propose replacement of providers in preview. (#2088) Providers with unknown properties are currently considered to require replacement. This was intended to indicate that we could not be sure whether or not replacement was reqiuired. Unfortunately, this was not a good user experience, as replacement would never be required at runtime. This caused quite a bit of confusion--never proposing replacement seems to be the better option. --- pkg/resource/plugin/provider_plugin.go | 30 ++------------------------ 1 file changed, 2 insertions(+), 28 deletions(-) diff --git a/pkg/resource/plugin/provider_plugin.go b/pkg/resource/plugin/provider_plugin.go index 67d8f16a8..fda2e2bb4 100644 --- a/pkg/resource/plugin/provider_plugin.go +++ b/pkg/resource/plugin/provider_plugin.go @@ -109,39 +109,13 @@ func (p *provider) DiffConfig(olds, news resource.PropertyMap) (DiffResult, erro // 1. Configuration differences in which all properties are known // 2. Configuration differences in which some new property is unknown. // - // Despite the fact that in both scenarios we know that configuration has changed, they differ in whether or not - // they return a diff result that indicates that the provider should be replaced (and thus require that any - // existing resource managed by the provider are also replaced). - // - // In the first case, we return a diff result that indicates that the provider _should not_ be replaced. We may - // encounter this scenario during any update or any preview in which all properties are known. Although this + // In both cases, we return a diff result that indicates that the provider _should not_ be replaced. Although this // decision is not conservative--indeed, the conservative decision would be to always require replacement of a // provider if any input has changed--we believe that it results in the best possible user experience for providers // that do not implement DiffConfig functionality. If we took the conservative route here, any change to a // provider's configuration (no matter how inconsequential) would cause all of its resources to be replaced. This // is clearly a bad experience, and differs from how things worked prior to first-class providers. - // - // In the second case, we return a diff result that indicates that the provider _should_ be replaced. This may - // occur during any preview, but will never occur during an update. This decision is conservative because we - // believe that it is unlikely that a provider property that may be unknown is very likely to be a property that - // is fundamental to the provider's ability to manage its existing resources. - // - // The different decisions we make here cause a bit of a sharp edge: a provider with unknown configuration during - // preview will appear to require replacement, but will never actually require replacement during an update, as by - // that point all of its configuration will necessarily be known. - - var replaceKeys []resource.PropertyKey - for k, v := range news { - // These are ensured during Check(). - contract.Assert(v.IsString() || v.IsComputed()) - - // As per the note above, any unknown properties require replacement. - if v.IsComputed() { - replaceKeys = append(replaceKeys, k) - } - } - - return DiffResult{Changes: DiffUnknown, ReplaceKeys: replaceKeys}, nil + return DiffResult{Changes: DiffUnknown, ReplaceKeys: nil}, nil } // getClient returns the client, and ensures that the target provider has been configured. This just makes it safer