Add Warning for Deprecated Modules in `Init` #34943

Maed223 · 2024-04-03T20:50:33Z

Target Release

1.8.x

Draft CHANGELOG entry

ENHANCEMENTS

terraform init: Displays a warning when deprecated modules are in use.

Description

All module deprecation info is consolidated into and displayed as a single warning upon running terraform init. This includes both cases of where the module does, or does not, need installation. Furthermore, the warning can be raised for both modules that are directly used in configuration, as well as any external dependencies they depend on.

Screenshot

Note that this screenshot includes both the root module as well as it's external dependencies

internal/initwd/module_install.go

internal/command/get.go

internal/initwd/module_install.go

internal/command/init.go

…e deprecation warnings

sebasslash

Things are looking great so far! Some items down below

sebasslash · 2024-05-08T20:38:00Z

internal/configs/module_deprecations.go

+	var deprecationList []*ModuleDeprecationDiagnosticExtraDeprecationItem
+	for _, modDeprecationInfo := range i.ModuleDeprecationInfos {
+		if modDeprecationInfo != nil && modDeprecationInfo.RegistryDeprecation != nil {
+			modDeprecation := fmt.Sprintf("\x1b[1mVersion %s of %s\x1b[0m", modDeprecationInfo.RegistryDeprecation.Version, modDeprecationInfo.SourceName)


Haven't tried this yet, but maybe we can leverage mitchellh/colorstring to format the colors rather than hardcoding the ANSI escape codes.

Suggested change

modDeprecation := fmt.Sprintf("\x1b[1mVersion %s of %s\x1b[0m", modDeprecationInfo.RegistryDeprecation.Version, modDeprecationInfo.SourceName)

msg := colorize.Color("[reset][bold]Version %s of %s[reset]")

modDeprecation := fmt.Sprintf(msg, modDeprecationInfo.RegistryDeprecation.Version, modDeprecationInfo.SourceName)

sebasslash · 2024-05-08T20:58:03Z

internal/configs/module_deprecations.go

+// as well as placed in the Diagnostic Extra for parsing for the SRO view in HCP Terraform
+func (i *WorkspaceDeprecationInfo) BuildDeprecationWarning() *hcl.Diagnostic {
+	modDeprecationStrings := []string{}
+	var deprecationList []*ModuleDeprecationDiagnosticExtraDeprecationItem


(optional 💅) I'm wondering if it's beneficial to pre-allocate this slice to have a capacity of len(i.ModuleDeprecationInfos), since we know the underlying array size won't exceed that. So it becomes:

deprecationList := make([]*ModuleDeprecationDiagnosticExtraDeprecationItem, 0, len(i.ModuleDeprecationInfo))

This may prevent extra memory allocations on append() for scenarios where we anticipate many deprecation diagnostics. Read about append() growth rates. See if you can also apply this to modDeprecationStrings

sebasslash · 2024-05-08T21:03:11Z

internal/initwd/module_install.go

+							found := false
+							for _, modProviderVersions := range resp.Modules {
+								for _, modVersions := range modProviderVersions.Versions {
+									vm, _ := version.NewVersion(modVersions.Version)
+									if vm.Equal(record.Version) {
+										moduleVersion = modVersions
+										found = true
+										break
+									}
+								}
+								if found {
+									break
+								}
+							}


Suggested change

found := false

for _, modProviderVersions := range resp.Modules {

for _, modVersions := range modProviderVersions.Versions {

vm, _ := version.NewVersion(modVersions.Version)

if vm.Equal(record.Version) {

moduleVersion = modVersions

found = true

break

}

}

if found {

break

}

}

found:

for _, modProviderVersions := range resp.Modules {

for _, modVersions := range modProviderVersions.Versions {

vm, _ := version.NewVersion(modVersions.Version)

if vm.Equal(record.Version) {

moduleVersion = modVersions

break found

}

}

}

Use label breaks instead 👍

sebasslash · 2024-05-08T21:04:55Z

internal/configs/module_deprecations.go

+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: BUSL-1.1
+
+package configs


Don't forget to add tests for module_deprecations_test.go 👍

nfagerlund

Several change requests inline; feel free to ask for more detail on anything.

I also have a couple big-picture questions for ya.

Have y'all started work on the TFC version of this warning? And where will TFC be receiving the details from? I ask because, terraform init doesn't have a json log format, and we suppress its output entirely unless it errors out with a non-zero exit code (Cf. core/components/terraform/step_terraform_init.go in tfc-agent), so TFC won't be able to learn about this from CLI. You likely already have plans to work around this, but I wanted to make sure I was raising the issue just in case
I think the current design of passing the deprecations info around as a return value from the module walk is viable, but for posterity's sake and to make sure you've considered the options — can you say a bit more about why this belongs in a separate return, rather than in a field on the configs.Config struct? Config is already a nested structure with child Configs for each module, so it sort of mirrors the separate structure you're building here.
We need tests for this behavior!! 😄

Also some follow-up smaller notes:

I'm not sure how we plan to support this in the stacks runtime — I think it might require changes in go-slug/sourcebundle. Any other tf-core stacks peeps want to chime in on that?

nfagerlund · 2024-05-08T21:17:13Z

internal/configs/config_build.go

@@ -111,7 +115,7 @@ func buildTestModules(root *Config, walker ModuleWalker) hcl.Diagnostics {
 				CallRange:         run.Module.DeclRange,
 			}

-			cfg, modDiags := loadModule(root, &req, walker)
+			cfg, modDiags, _ := loadModule(root, &req, walker)


Oh right 🤔 I guess we have no way to know if the root module is deprecated, because we didn't fetch it from its source.

nfagerlund · 2024-05-08T21:18:34Z

internal/configs/config_build.go

@@ -253,6 +263,8 @@ func rebaseChildModule(cfg *Config, root *Config) {
 	cfg.Root = root
 }

+// refactor this into it's own file, doesn't make sense to place this here


This looks like detritus from a previous iteration; please remove.

nfagerlund · 2024-05-08T21:50:28Z

internal/initwd/module_install.go

+							for _, modProviderVersions := range resp.Modules {
+								for _, modVersions := range modProviderVersions.Versions {
+									vm, _ := version.NewVersion(modVersions.Version)
+									if vm.Equal(record.Version) {


Haha, this freaked me out for a minute, but turns out that .Equal explicitly expects to maybe be called on a nil receiver. Ok!

nfagerlund · 2024-05-08T22:32:06Z

internal/command/views/json/diagnostic.go

@@ -149,6 +150,7 @@ func NewDiagnostic(diag tfdiags.Diagnostic, sources map[string][]byte) *Diagnost
 		Summary:  desc.Summary,
 		Detail:   desc.Detail,
 		Address:  desc.Address,
+		Extra:    diag.ExtraInfo(),


🚨 This makes me nervous!

As you saw in the tests that this broke, the existing ExtraInfo() types do not expect that they will be exposed to the outside world, and might create nonsense if they do get exposed. ("extra": true? huh???)

I think you might want to

Create a new tfdiags.PublicExtraInfo interface type (with one do-nothing function that types must implement).

Grab the value of diag.ExtraInfo() and do a type-check on it, and conditionally add it to the output value only if it implements PublicExtraInfo.

Implement PublicExtraInfo for ModuleDeprecationDiagnosticExtra.

That way any existing extra types stay unexposed.

nfagerlund · 2024-05-08T22:53:49Z

internal/command/get.go

@@ -42,7 +42,6 @@ func (c *GetCommand) Run(args []string) int {
 	}

 	path = c.normalizePath(path)
-


This looks like detritus from a previous iteration; plz remove.

nfagerlund · 2024-05-08T23:49:27Z

internal/configs/module_deprecations.go

+	var deprecationList []*ModuleDeprecationDiagnosticExtraDeprecationItem
+	for _, modDeprecationInfo := range i.ModuleDeprecationInfos {
+		if modDeprecationInfo != nil && modDeprecationInfo.RegistryDeprecation != nil {
+			modDeprecation := fmt.Sprintf("\x1b[1mVersion %s of %s\x1b[0m", modDeprecationInfo.RegistryDeprecation.Version, modDeprecationInfo.SourceName)


👀 Mmmmmm, this can't fly — unconditionally embedding ansi control codes into the text means this can never be a good citizen when -no-color is specified.

On the CLI, diagnostics end up going through View.Diagnostics. So, you can use colorize codes in the text that will get stripped out when it's in no-color mode. In this case, you'd want to replace the ansi codes with [bold]something[reset].

nfagerlund · 2024-05-08T23:58:32Z

internal/configs/module_deprecations.go

+	for _, deprecationString := range modDeprecationStrings {
+		deprecationsMessage += deprecationString + "\n\n"
+	}


I think you want strings.Join here.

nfagerlund · 2024-05-09T00:06:32Z

internal/command/views/json/diagnostic.go

@@ -38,6 +38,7 @@ type Diagnostic struct {
 	Address  string             `json:"address,omitempty"`
 	Range    *DiagnosticRange   `json:"range,omitempty"`
 	Snippet  *DiagnosticSnippet `json:"snippet,omitempty"`
+	Extra    interface{}        `json:"extra,omitempty"`


A change to this externally-facing json format will require an update to its docs page (https://developer.hashicorp.com/terraform/cli/commands/validate#json), and possibly a bump to its minor version number.

(But, I asked around about how seriously we take the minor version guarantees in these json formats, and the answer was inconclusive. We do at least want it documented, though.)

nfagerlund · 2024-05-09T00:32:24Z

internal/configs/module_deprecations.go

+		deprecationsMessage += deprecationString + "\n\n"
+	}
+
+	return &hcl.Diagnostic{


I've got a modest architectural suggestion for you here — instead of re-using the hcl.Diagnostic type here, what do you think about making your own DeprecationsDiagnostic struct, shoving the flattened deprecationList (and a final boolean hasDeprecations result) into it, and implementing tfdiags.Diagnostic on it?

I think it could yield some cleaner-feeling code, but it's not something I'd necessarily insist on.

vercel bot deployed to Preview April 3, 2024 20:55 View deployment

vercel bot deployed to Preview April 4, 2024 13:40 View deployment

vercel bot deployed to Preview April 4, 2024 21:43 View deployment

vercel bot deployed to Preview April 5, 2024 20:22 View deployment

vercel bot deployed to Preview April 5, 2024 22:21 View deployment

vercel bot deployed to Preview April 7, 2024 17:35 View deployment

vercel bot deployed to Preview April 9, 2024 19:04 View deployment

vercel bot deployed to Preview April 9, 2024 21:03 View deployment

Maed223 force-pushed the TF-12601/deprecated-module-warnings branch from 6b348b4 to ae4207f Compare April 9, 2024 21:25

vercel bot deployed to Preview April 9, 2024 21:29 View deployment

vercel bot deployed to Preview April 9, 2024 21:50 View deployment

vercel bot deployed to Preview April 11, 2024 21:06 View deployment

vercel bot deployed to Preview April 12, 2024 23:44 View deployment

vercel bot deployed to Preview April 15, 2024 21:18 View deployment

vercel bot deployed to Preview April 16, 2024 21:02 View deployment

vercel bot deployed to Preview April 17, 2024 18:27 View deployment

vercel bot had a problem deploying to Preview April 18, 2024 21:10 Failure

vercel bot had a problem deploying to Preview April 18, 2024 21:18 Failure

uturunku1 reviewed Apr 19, 2024

View reviewed changes

internal/initwd/module_install.go Outdated Show resolved Hide resolved

uturunku1 reviewed Apr 19, 2024

View reviewed changes

internal/initwd/module_install.go Show resolved Hide resolved

uturunku1 reviewed Apr 19, 2024

View reviewed changes

internal/initwd/module_install.go Outdated Show resolved Hide resolved

uturunku1 reviewed Apr 19, 2024

View reviewed changes

internal/command/get.go Outdated Show resolved Hide resolved

uturunku1 reviewed Apr 19, 2024

View reviewed changes

internal/initwd/module_install.go Outdated Show resolved Hide resolved

uturunku1 reviewed Apr 19, 2024

View reviewed changes

internal/command/init.go Outdated Show resolved Hide resolved

vercel bot deployed to Preview April 19, 2024 19:27 View deployment

vercel bot deployed to Preview April 19, 2024 19:45 View deployment

vercel bot deployed to Preview April 19, 2024 20:30 View deployment

Maed223 force-pushed the TF-12601/deprecated-module-warnings branch from b9107c0 to 5471fdf Compare April 19, 2024 20:41

vercel bot deployed to Preview April 19, 2024 20:45 View deployment

vercel bot deployed to Preview April 19, 2024 20:52 View deployment

vercel bot deployed to Preview May 6, 2024 15:19 View deployment

Maed223 force-pushed the TF-12601/deprecated-module-warnings branch from 0e24d07 to f27dad2 Compare May 6, 2024 16:36

vercel bot deployed to Preview May 6, 2024 16:39 View deployment

Maed223 force-pushed the TF-12601/deprecated-module-warnings branch from f27dad2 to 18df73f Compare May 6, 2024 19:55

vercel bot deployed to Preview May 6, 2024 19:59 View deployment

vercel bot deployed to Preview May 6, 2024 20:55 View deployment

vercel bot deployed to Preview May 6, 2024 21:06 View deployment

Maed223 force-pushed the TF-12601/deprecated-module-warnings branch from a9d0a9c to 1f19a2b Compare May 6, 2024 21:06

vercel bot deployed to Preview May 6, 2024 21:10 View deployment

Maed223 force-pushed the TF-12601/deprecated-module-warnings branch from 1f19a2b to b517651 Compare May 6, 2024 21:21

vercel bot deployed to Preview May 6, 2024 21:25 View deployment

Update ModuleVersion to include new deprecated fields

3f3bef1

Maed223 force-pushed the TF-12601/deprecated-module-warnings branch 2 times, most recently from c809c74 to c8d198a Compare May 7, 2024 17:00

Maed223 added 10 commits May 7, 2024 13:03

test fixes

2c96dd9

include warnings on using deprecated modules w/ mocked data

6d4d452

test fixes

ac44e40

Return ModuleDeprecationInfo in any case of module source, consolidat…

971819a

…e deprecation warnings

updated with feedback from @uturunku1

5636d1f

update to warning design

758676c

remove mocking for module deprecations

8336e91

Add extra field for mod deprecation warning

e43c712

test fixes

e70969d

Add deprecation info in diagnostic extra field for SRO parsing

a79e38c

Maed223 force-pushed the TF-12601/deprecated-module-warnings branch from c8d198a to a79e38c Compare May 7, 2024 17:03

vercel bot deployed to Preview May 7, 2024 17:11 View deployment

uturunku1 requested review from Uk1288 and uturunku1 May 8, 2024 16:48

sebasslash requested changes May 8, 2024

View reviewed changes

nfagerlund requested changes May 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Warning for Deprecated Modules in `Init` #34943

Add Warning for Deprecated Modules in `Init` #34943

Maed223 commented Apr 3, 2024 •

edited

sebasslash left a comment

sebasslash May 8, 2024

sebasslash May 8, 2024

sebasslash May 8, 2024

sebasslash May 8, 2024

nfagerlund left a comment •

edited

nfagerlund May 8, 2024

nfagerlund May 8, 2024

nfagerlund May 8, 2024

nfagerlund May 8, 2024

nfagerlund May 8, 2024

nfagerlund May 8, 2024

nfagerlund May 8, 2024

nfagerlund May 9, 2024

nfagerlund May 9, 2024

	modDeprecation := fmt.Sprintf("\x1b[1mVersion %s of %s\x1b[0m", modDeprecationInfo.RegistryDeprecation.Version, modDeprecationInfo.SourceName)
	msg := colorize.Color("[reset][bold]Version %s of %s[reset]")
	modDeprecation := fmt.Sprintf(msg, modDeprecationInfo.RegistryDeprecation.Version, modDeprecationInfo.SourceName)

		@@ -42,7 +42,6 @@ func (c *GetCommand) Run(args []string) int {
		}

		path = c.normalizePath(path)

Add Warning for Deprecated Modules in Init #34943

Are you sure you want to change the base?

Add Warning for Deprecated Modules in Init #34943

Conversation

Maed223 commented Apr 3, 2024 • edited

Target Release

Draft CHANGELOG entry

ENHANCEMENTS

Description

Screenshot

sebasslash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfagerlund left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add Warning for Deprecated Modules in `Init` #34943

Add Warning for Deprecated Modules in `Init` #34943

Maed223 commented Apr 3, 2024 •

edited

nfagerlund left a comment •

edited