Merge pull request #35264 from hashicorp/revert-35231-TF-12601/deprecated-module-version-warns-rev2

Author: brandonc

internal/command/views/json/diagnostic.go

@@ -38,7 +38,6 @@ type Diagnostic struct {
 	Address  string             `json:"address,omitempty"`
 	Range    *DiagnosticRange   `json:"range,omitempty"`
 	Snippet  *DiagnosticSnippet `json:"snippet,omitempty"`
-	Extra    any                `json:"extra,omitempty"`
 }
 
 // Pos represents a position in the source code.
@@ -145,18 +144,11 @@ func NewDiagnostic(diag tfdiags.Diagnostic, sources map[string][]byte) *Diagnost
 
 	desc := diag.Description()
 
-	extra := diag.ExtraInfo()
-	var publicExtra tfdiags.PublicExtraInfo
-	if e, ok := extra.(tfdiags.PublicExtraInfo); ok {
-		publicExtra = e
-	}
-
 	diagnostic := &Diagnostic{
 		Severity: sev,
 		Summary:  desc.Summary,
 		Detail:   desc.Detail,
 		Address:  desc.Address,
-		Extra:    publicExtra,
 	}
 
 	sourceRefs := diag.Source()

internal/initwd/module_install.go

@@ -49,28 +49,6 @@ type moduleVersion struct {
 	version string
 }
 
-// TypeModuleVersionDeprecation is a constant of TypeDiagnosticExtra indicating
-// that a Warning is related to a deprecated module version is in use.
-type TypeDiagnosticExtra string
-
-const TypeModuleVersionDeprecation TypeDiagnosticExtra = "module_version_deprecation"
-
-// ModuleVersionDeprecationDiagnosticExtra holds the diagnostic information
-// about the deprecation of a module version. This ends up being serialized as extra data within a
-// diagnostic and should be considered public API.
-type ModuleVersionDeprecationDiagnosticExtra struct {
-	Type               TypeDiagnosticExtra `json:"type"`
-	Version            string              `json:"version"`
-	SourceAddr         string              `json:"source_name"`
-	DeprecationMessage string              `json:"deprecation_message"`
-	Link               string              `json:"link"`
-}
-
-// IsPublic confirms the visibility of the extra field in the public API.
-func (m ModuleVersionDeprecationDiagnosticExtra) IsPublic() {
-	// NOP
-}
-
 func NewModuleInstaller(modsDir string, loader *configload.Loader, reg *registry.Client) *ModuleInstaller {
 	return &ModuleInstaller{
 		modsDir:                 modsDir,
@@ -276,32 +254,6 @@ func (i *ModuleInstaller) moduleInstallWalker(ctx context.Context, manifest mods
 					}
 
 					log.Printf("[TRACE] ModuleInstaller: Module installer: %s %s already installed in %s", key, record.Version, record.Dir)
-
-					// Checking for module deprecations in the case no new module versions need installation
-					if addr, isRegistryModule := req.SourceAddr.(addrs.ModuleSourceRegistry); isRegistryModule {
-						regClient := i.reg
-
-						regsrcAddr := regsrc.ModuleFromRegistryPackageAddr(addr.Package)
-						resp, err := regClient.ModuleVersions(ctx, regsrcAddr)
-						if err != nil {
-							log.Printf("[WARN] Deprecation for %s could not be checked: call to registry failed: %v", addr.Package.String(), err)
-
-						} else {
-						found:
-							for _, modProviderVersions := range resp.Modules {
-								for _, modVersion := range modProviderVersions.Versions {
-									vm, _ := version.NewVersion(modVersion.Version)
-									if vm.Equal(record.Version) {
-										if modVersion.Deprecation != nil {
-											diags = append(diags, buildModuleVersionDeprecationWarning(modVersion, req, addr.Package.String()))
-										}
-										break found
-									}
-								}
-							}
-						}
-					}
-
 					return mod, record.Version, diags
 				}
 			}
@@ -528,8 +480,7 @@ func (i *ModuleInstaller) installRegistryModule(ctx context.Context, req *config
 
 	modMeta := resp.Modules[0]
 
-	var latestMatch *response.ModuleVersion
-	var latestMatchVersion *version.Version
+	var latestMatch *version.Version
 	var latestVersion *version.Version
 	for _, mv := range modMeta.Versions {
 		v, err := version.NewVersion(mv.Version)
@@ -594,9 +545,8 @@ func (i *ModuleInstaller) installRegistryModule(ctx context.Context, req *config
 		}
 
 		if req.VersionConstraint.Required.Check(v) {
-			if latestMatch == nil || v.GreaterThan(latestMatchVersion) {
-				latestMatch = mv
-				latestMatchVersion = v
+			if latestMatch == nil || v.GreaterThan(latestMatch) {
+				latestMatch = v
 			}
 		}
 	}
@@ -621,27 +571,23 @@ func (i *ModuleInstaller) installRegistryModule(ctx context.Context, req *config
 		return nil, nil, diags
 	}
 
-	if latestMatch.Deprecation != nil {
-		diags = diags.Append(buildModuleVersionDeprecationWarning(latestMatch, req, addr.Package.String()))
-	}
-
 	// Report up to the caller that we're about to start downloading.
-	hooks.Download(key, packageAddr.String(), latestMatchVersion)
+	hooks.Download(key, packageAddr.String(), latestMatch)
 
 	// If we manage to get down here then we've found a suitable version to
 	// install, so we need to ask the registry where we should download it from.
 	// The response to this is a go-getter-style address string.
 
 	// first check the cache for the download URL
-	moduleAddr := moduleVersion{module: packageAddr, version: latestMatchVersion.String()}
+	moduleAddr := moduleVersion{module: packageAddr, version: latestMatch.String()}
 	if _, exists := i.registryPackageSources[moduleAddr]; !exists {
-		realAddrRaw, err := reg.ModuleLocation(ctx, regsrcAddr, latestMatchVersion.String())
+		realAddrRaw, err := reg.ModuleLocation(ctx, regsrcAddr, latestMatch.String())
 		if err != nil {
-			log.Printf("[ERROR] %s from %s %s: %s", key, addr, latestMatchVersion, err)
+			log.Printf("[ERROR] %s from %s %s: %s", key, addr, latestMatch, err)
 			diags = diags.Append(&hcl.Diagnostic{
 				Severity: hcl.DiagError,
 				Summary:  "Error accessing remote module registry",
-				Detail:   fmt.Sprintf("Failed to retrieve a download URL for %s %s from %s: %s", addr, latestMatchVersion, hostname, err),
+				Detail:   fmt.Sprintf("Failed to retrieve a download URL for %s %s from %s: %s", addr, latestMatch, hostname, err),
 			})
 			return nil, nil, diags
 		}
@@ -650,7 +596,7 @@ func (i *ModuleInstaller) installRegistryModule(ctx context.Context, req *config
 			diags = diags.Append(&hcl.Diagnostic{
 				Severity: hcl.DiagError,
 				Summary:  "Invalid package location from module registry",
-				Detail:   fmt.Sprintf("Module registry %s returned invalid source location %q for %s %s: %s.", hostname, realAddrRaw, addr, latestMatchVersion, err),
+				Detail:   fmt.Sprintf("Module registry %s returned invalid source location %q for %s %s: %s.", hostname, realAddrRaw, addr, latestMatch, err),
 			})
 			return nil, nil, diags
 		}
@@ -665,15 +611,15 @@ func (i *ModuleInstaller) installRegistryModule(ctx context.Context, req *config
 			diags = diags.Append(&hcl.Diagnostic{
 				Severity: hcl.DiagError,
 				Summary:  "Invalid package location from module registry",
-				Detail:   fmt.Sprintf("Module registry %s returned invalid source location %q for %s %s: must be a direct remote package address.", hostname, realAddrRaw, addr, latestMatchVersion),
+				Detail:   fmt.Sprintf("Module registry %s returned invalid source location %q for %s %s: must be a direct remote package address.", hostname, realAddrRaw, addr, latestMatch),
 			})
 			return nil, nil, diags
 		}
 	}
 
 	dlAddr := i.registryPackageSources[moduleAddr]
 
-	log.Printf("[TRACE] ModuleInstaller: %s %s %s is available at %q", key, packageAddr, latestMatchVersion, dlAddr.Package)
+	log.Printf("[TRACE] ModuleInstaller: %s %s %s is available at %q", key, packageAddr, latestMatch, dlAddr.Package)
 
 	err := fetcher.FetchPackage(ctx, instPath, dlAddr.Package.String())
 	if errors.Is(err, context.Canceled) {
@@ -735,14 +681,14 @@ func (i *ModuleInstaller) installRegistryModule(ctx context.Context, req *config
 	// Note the local location in our manifest.
 	manifest[key] = modsdir.Record{
 		Key:        key,
-		Version:    latestMatchVersion,
+		Version:    latestMatch,
 		Dir:        modDir,
 		SourceAddr: req.SourceAddr.String(),
 	}
 	log.Printf("[DEBUG] Module installer: %s installed at %s", key, modDir)
-	hooks.Install(key, latestMatchVersion, modDir)
+	hooks.Install(key, latestMatch, modDir)
 
-	return mod, latestMatchVersion, diags
+	return mod, latestMatch, diags
 }
 
 func (i *ModuleInstaller) installGoGetterModule(ctx context.Context, req *configs.ModuleRequest, key string, instPath string, manifest modsdir.Manifest, hooks ModuleInstallHooks, fetcher *getmodules.PackageFetcher) (*configs.Module, hcl.Diagnostics) {
@@ -990,27 +936,3 @@ func splitAddrSubdir(addr addrs.ModuleSource) (string, string) {
 		return addr.String(), ""
 	}
 }
-
-func buildModuleVersionDeprecationWarning(modVersion *response.ModuleVersion, req *configs.ModuleRequest, packageAddr string) *hcl.Diagnostic {
-	var additionalInfo []string
-	if modVersion.Deprecation.Reason != "" {
-		additionalInfo = append(additionalInfo, modVersion.Deprecation.Reason)
-	}
-	if modVersion.Deprecation.Link != "" {
-		additionalInfo = append(additionalInfo, fmt.Sprintf("More information: %s", modVersion.Deprecation.Link))
-	}
-	detail := strings.Join(additionalInfo, "\n\n")
-	return &hcl.Diagnostic{
-		Severity: hcl.DiagWarning,
-		Summary:  fmt.Sprintf("Module version %s of %s is deprecated", modVersion.Version, packageAddr),
-		Detail:   detail,
-		Subject:  req.CallRange.Ptr(),
-		Extra: &ModuleVersionDeprecationDiagnosticExtra{
-			Type:               TypeModuleVersionDeprecation,
-			Version:            modVersion.Version,
-			SourceAddr:         packageAddr,
-			DeprecationMessage: modVersion.Deprecation.Reason,
-			Link:               modVersion.Deprecation.Link,
-		},
-	}
-}

internal/initwd/module_install_test.go

@@ -25,7 +25,6 @@ import (
 	"github.com/hashicorp/terraform/internal/configs/configload"
 	"github.com/hashicorp/terraform/internal/copy"
 	"github.com/hashicorp/terraform/internal/registry"
-	"github.com/hashicorp/terraform/internal/registry/response"
 	"github.com/hashicorp/terraform/internal/tfdiags"
 
 	_ "github.com/hashicorp/terraform/internal/logging"
@@ -596,70 +595,6 @@ func TestLoaderInstallModules_registry(t *testing.T) {
 
 }
 
-func TestLoaderInstallModules_registry_deprecated(t *testing.T) {
-	fixtureDir := filepath.Clean("testdata/registry-module-from-test")
-	tmpDir, done := tempChdir(t, fixtureDir)
-	// the module installer runs filepath.EvalSymlinks() on the destination
-	// directory before copying files, and the resultant directory is what is
-	// returned by the install hooks. Without this, tests could fail on machines
-	// where the default temp dir was a symlink.
-	dir, err := filepath.EvalSymlinks(tmpDir)
-	if err != nil {
-		t.Error(err)
-	}
-
-	defer done()
-
-	hooks := &testInstallHooks{}
-	modulesDir := filepath.Join(dir, ".terraform/modules")
-
-	loader, close := configload.NewLoaderForTests(t)
-	defer close()
-
-	inst := NewModuleInstaller(modulesDir, loader, registry.NewClient(nil, nil))
-
-	// Avoid actual registry lookup by populating registry cache
-	packageAddr := addrs.ModuleRegistryPackage{
-		Host:         svchost.Hostname("registry.terraform.io"),
-		Namespace:    "hashicorp",
-		Name:         "module-installer-acctest",
-		TargetSystem: "aws",
-	}
-
-	inst.registryPackageVersions[packageAddr] = &response.ModuleVersions{
-		Modules: []*response.ModuleProviderVersions{
-			{
-				Source: "",
-				Versions: []*response.ModuleVersion{
-					{
-						Version:    "0.0.1",
-						Root:       response.VersionSubmodule{},
-						Submodules: []*response.VersionSubmodule{},
-						Deprecation: &response.Deprecation{
-							Reason: "This module version is deprecated",
-							Link:   "https://example.com/deprecation",
-						},
-					},
-				},
-			},
-		},
-	}
-
-	_, diags := inst.InstallModules(context.Background(), dir, "tests", false, false, hooks)
-
-	if !diags.HasWarnings() {
-		t.Fatal("expected warning")
-	} else {
-		assertDiagnosticCount(t, diags, 1)
-		assertDiagnosticSummary(t, diags, "Module version 0.0.1 of registry.terraform.io/hashicorp/module-installer-acctest/aws is deprecated")
-
-		wantDetail := "This module version is deprecated\n\nMore information: https://example.com/deprecation"
-		if diags[0].Description().Detail != wantDetail {
-			t.Errorf("wrong deprecation detail\nwant: %s\ngot: %s", wantDetail, diags[0].Description().Detail)
-		}
-	}
-}
-
 func TestLoaderInstallModules_goGetter(t *testing.T) {
 	if os.Getenv("TF_ACC") == "" {
 		t.Skip("this test accesses github.com; set TF_ACC=1 to run it")
@@ -1033,7 +968,7 @@ func assertDiagnosticCount(t *testing.T, diags tfdiags.Diagnostics, want int) bo
 	if len(diags) != want {
 		t.Errorf("wrong number of diagnostics %d; want %d", len(diags), want)
 		for _, diag := range diags {
-			t.Logf("- %#v", diag.Description().Summary)
+			t.Logf("- %#v", diag)
 		}
 		return true
 	}

internal/registry/response/module_versions.go

@@ -20,16 +20,9 @@ type ModuleProviderVersions struct {
 // ModuleVersion is the output metadata for a given version needed by CLI to
 // resolve candidate versions to satisfy requirements.
 type ModuleVersion struct {
-	Version     string              `json:"version"`
-	Root        VersionSubmodule    `json:"root"`
-	Submodules  []*VersionSubmodule `json:"submodules"`
-	Deprecation *Deprecation        `json:"deprecation"`
-}
-
-// Deprecation holds the user provided reason and link for the specific module version deprecation
-type Deprecation struct {
-	Reason string `json:"reason"`
-	Link   string `json:"link"`
+	Version    string              `json:"version"`
+	Root       VersionSubmodule    `json:"root"`
+	Submodules []*VersionSubmodule `json:"submodules"`
 }
 
 // VersionSubmodule is the output metadata for a submodule within a given

internal/tfdiags/diagnostic.go

@@ -65,8 +65,3 @@ type FromExpr struct {
 	Expression  hcl.Expression
 	EvalContext *hcl.EvalContext
 }
-
-// PublicExtraInfo is an interface for marking Extra field that contain public extra information
-type PublicExtraInfo interface {
-	IsPublic()
-}

website/docs/cli/commands/validate.mdx

@@ -178,9 +178,6 @@ The nested objects in `diagnostics` have the following properties:
     which may be useful in understanding the source of a diagnostic in a
     complex expression. These expression value objects are described below.
 
-  - `extra` (object): An optional object that serves as an extension point for additional 
-    machine-readable information about the problem.
-
 ### Source Position
 
 A source position object, as used in the `range` property of a diagnostic