Warn on bundled plugin usage #12495
Warn on bundled plugin usage #12495
Conversation
There was a problem hiding this comment.
This looks great. I left a few suggestions on the text to clarify what a bundle plugin is and to make the call to action a bit clearer to the user. Also, as discussed lets split the warning for JSON and HCL2 users to reduce the scroll back and possible confusion for users just using legacy JSON templates.
lbajolet-hashicorp
force-pushed
the
warn_on_bundled_plugin_usage
branch
7 times, most recently
from
9aef8cc to
4c143b3
Compare
lbajolet-hashicorp
changed the base branch from
main
to
hcl2_warn_once_on_uninstalled_plugins
Since the `discoverExternalComponents' function was defined but not called anywhere, it is dead code, and can be safely removed from the codebase.
lbajolet-hashicorp
force-pushed
the
warn_on_bundled_plugin_usage
branch
from
4c143b3 to
4634d39
Compare
packer/run_interfaces.go
Outdated
| @@ -45,6 +45,9 @@ type InitializeOptions struct { | |||
| // run a build we will start the builds and then the core of Packer handles | |||
| // execution. | |||
| type Handler interface { | |||
| // PreInitialize is used only for HCL2 templates, and loads required | |||
| // plugins if specified. | |||
| PreInitialize() hcl.Diagnostics | |||
There was a problem hiding this comment.
I like the interface approach but this PreInitialize seems a bit confusing and it hides a bit what is actually happening within this method for each Starter. Especially since PreInitialize is a no op for legacy JSON starter. What if you take the same approach but instead create a 1 function interface.
type BundlePluginInitializer interface {
InitializeBundlePlugins() hcl.Diagnostics
}Then have the Handler embed this interface
type Handler interface {
BundlePluginIntializer
// all other interfaces
}The one function interface is an approach we've take for the commands so it wold be inline with what we already do. The code remains the same but it removes this idea of a PreInitialize step which I can see being confusing and possibly overloaded.
There was a problem hiding this comment.
This makes sense yep, I wasn't a big fan of the PreInitialize name tbh, so I like the alternative :)
I'll reroll this ASAP
There was a problem hiding this comment.
Done! I went with a slightly different name in the end since the purpose of this step is to load the plugin binaries specified in the required_plugins block, so I called it PluginBinaryDetector, and for the function, I reused the DetectPluginBinaries name since that's what it was called in the first place.
There was a problem hiding this comment.
Overall this looks good to me and works as expected. I found the PreInitialize method a bit vague and suggested a different name to help make it immediately clear what is being done. This is code that will be removed once plugins are removed so having a name reflect what is being done for bundled plugins feels a bit more readable to me.
If you have other thoughts for PreInitialize that I'm not seeing or may have overlooked please let me know.
Since bundled plugins will be removed in an upcoming version of Packer, this commit adds a new warning message whenever a template uses one such plugin. This warning has been implemented on build, validate, console and the inspect subcommands. In addition to warning about the upcoming change and potential issue this will cause, this warning message proposes solutions to the user so they know what they'll have to do in order not to rely on those bundled plugins later.
lbajolet-hashicorp
force-pushed
the
warn_on_bundled_plugin_usage
branch
from
4634d39 to
38f95d3
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 issues. |
Since bundled plugins will be removed in an upcoming version of Packer, this commit adds a new warning message whenever a template uses one such plugin.
This warning has been implemented on build, validate, console and the inspect subcommands.
In addition to warning about the upcoming change and potential issue this will cause, this warning message proposes solutions to the user so they know what they'll have to do in order not to rely on those bundled plugins later.