-
Notifications
You must be signed in to change notification settings - Fork 9.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Warning for Deprecated Modules in Init
#34943
Conversation
6b348b4
to
ae4207f
Compare
b9107c0
to
5471fdf
Compare
1788cfa
to
63bd458
Compare
a233dd0
to
420ec73
Compare
e385286
to
1693bbd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some early questions that will help me get my bearings on this PR
cfg, instDiags, workspaceDeprecations := i.installDescendentModules(rootMod, manifest, walker, installErrsOnly) | ||
diags = append(diags, instDiags...) | ||
if workspaceDeprecations.HasDeprecations() { | ||
moduleDeprecationWarning := workspaceDeprecations.BuildDeprecationWarning() | ||
diags = diags.Append(moduleDeprecationWarning) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious: Why couldn't the ModuleInstaller itself build the deprecation warnings and add them to the instDiags that are already returned? It would seem that would simplify the interface greatly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My rationale is that they are being appended to diags along with instDiags, without further differentiation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comes from the requirement that we consolidate the deprecations into a single warning, as well as having a means of showing the hierarchy in which a deprecated module sits (i.e. the (Root: a -> b -> c)
parts of the warning).
@@ -45,6 +45,7 @@ require ( | |||
github.com/manicminer/hamilton-autorest v0.2.0 // indirect | |||
github.com/mattn/go-colorable v0.1.13 // indirect | |||
github.com/mattn/go-isatty v0.0.20 // indirect | |||
github.com/mitchellh/colorstring v0.0.0-20190213212951-d06e56a500db // indirect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These backend go mod changes look out of place because no other changes are in the package. What's going on with them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something I'm not 100% sure on, but it seems to me that change was needed since I added the dependency to configs
package, which is somehow a dependency of backend
package. Without this change I was getting failures in "Code Consistency Checks" test on CI, as well as some failing unit tests due to the missing presence of the dependency. This change and it's related changes are the result of running make syncdeps
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took some edits to how the Detail
of diagnostics are handled in the format
package, but was able to remove the direct colorization in the configs
package that subsequently removes the need for these changes.
Included here 7b3e8a1
Otherwise dropped the previous commit that included the go.mod
and go.sum
changes
@nfagerlund– just to address some of your points ✍️ TFE Version of this warning
Passing deprecations as a return value separate from
Changes in go-slug/sourcebundle
|
ce53911
to
7b3e8a1
Compare
…it colorizing of diags details & abiding to -no-color
7b3e8a1
to
6ff57ce
Compare
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Jira Ticket
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