packer: log plugins used and their version/path #12567
Conversation
| "golang.org/x/mod/modfile" | ||
| ) | ||
|
|
||
| //go:embed go.mod |
There was a problem hiding this comment.
It's nice to see how simple this turned out to be but it feels like a short lived change given that their versions haven't changed in the past 4 releases and that we will be removing bundle plugins soon. But I do think logging the versions for external plugins makes sense.
There was a problem hiding this comment.
In principle I agree with you, so that leaves us with a couple alternatives:
- We don't show bundled plugin usage: this is relatively straightforward, as we can remove this code. What I don't like is that it doesn't reflect the true plugin usage (though we do see the warning for bundled plugins which may lead us on which plugin is used, but isn't perfect, especially for JSON).
- We track bundled plugins, without the version info: doing so means that we need to infer which plugins are loaded, and track their version as bundled. This may not result in code that is as separated as this one though, as we'll probably need to hack something elsewhere in the codebase.
- This solution.
Given those alternatives, I know I would prefer option 3 since the work is already done, the code is straightforward, and concentrated (besides the call to this in main), so when we remove bundled plugins, this will be equally easy to cleanup (at least compared to solution 2).
Let me know what you think on that, I would have liked to include this in 1.9.3, but we can push it to a later version if there are reservations on this implementation.
|
|
||
| tmpl := c.Template | ||
| if tmpl == nil { | ||
| panic("No template parsed. This is a Packer bug which should be reported, please open an issue on the project's issue tracker.") |
There was a problem hiding this comment.
This seems like a pattern but I think throwing a panic for failures that are not critical to the execution of Packer can be more a blocker to the user than helpful. Especially, if there are unknown cases that cause an expected failure. For example the panics happening in the tests runs.
If we can't get this information for logging or displaying a warning to a user we should log the failure and not stop the build.
There was a problem hiding this comment.
I agree with you, but at this time, if the template isn't properly parsed, this means that we've changed something in the logic, and we should be made aware ASAP, not later when we investigate why we don't see a warning/error anymore, and we notice this log.
At least that's my reasoning, and why I think some sanity-check panics like these are still relevant.
That being said, I'm not against the idea of changing this to a log/UI.Error, let me know which makes sense in your opinion.
As maintainers of Packer and plugins, we ask our users to provide us with the versions of Packer and the plugins they use when they open an issue for us to review. This is often misunderstood, and may be hidden in the logs, making it harder for all of us to understand where to look. This commit adds a log statement that reports the list of plugins used, their version, and the path they've been loaded from.
lbajolet-hashicorp
force-pushed
the
log_used_plugins
branch
from
ad5a2d5
to
8232633
Compare
|
Converting this to a draft for now as we discussed bundled plugin removal and plugin loading changes. |
nywilken
added
stage/thinking
As maintainers of Packer and plugins, we ask our users to provide us with the versions of Packer and the plugins they use when they open an issue for us to review.
This is often misunderstood, and may be hidden in the logs, making it harder for all of us to understand where to look.
This commit adds a log statement that reports the list of plugins used, their version, and the path they've been loaded from.