Skip to content
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

[HPR-1138] Limit recursive plugin scanning to Packer plugin paths set by PACKER_CONFIG_DIR or PACKER_PLUGIN_PATH #12414

Merged
merged 3 commits into from
Jun 22, 2023

Conversation

nywilken
Copy link
Contributor

@nywilken nywilken commented May 11, 2023

Packer will try to discover installed plugins in all of the directories
defined by packer.KnowPluginFolders. In a previous release logic was
added to scan nested directories in order to load plugins installed by
packer plugins install. This change resulted in a nested directory
scan for each folder within the KnownPluginFolders slice.

This change reduces the nested directory scan to only the directories
where plugins would have been installed using packer plugins install

  • Fix executable directory path
  • Reduce the number of nest plugin scans

Closes: #12405
Closes: #12352

This is sort of a breaking change as it changes how plugins are loaded but also not really as I think it works like this now but might be incorrectly documented 😄


TODO

  • Fix broken tests
  • Add new test for validating plugin scanning behavior
  • Update documentation to reflect how plugins are scanned and loaded. The current docs are a little out of date. Opening in a separate PR.

@nywilken nywilken changed the title nywilken/plugin scanning [HPR-1138] Limit recursive plugin scanning to Packer plugin paths set by PACKER_CONFIG_DIR or PACKER_PLUGIN_PATH May 24, 2023
@nywilken nywilken marked this pull request as ready for review May 24, 2023 21:04
@nywilken nywilken requested a review from a team as a code owner May 24, 2023 21:04
Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I'm not super familiar with this part of the code, but it looks legit to me!


pluginPaths, err = c.discoverSingle(filepath.Join(path, "*", "*", "*", fmt.Sprintf("packer-plugin-*%s", binInstallOpts.FilenameSuffix())))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I presume this was the reason for the high number of syscalls? Globbing with this pattern will try to unravel a ton of subpaths before eventually finding the plugin(s) itself?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would check out with the history of the problem, since this change was introduced with v1.8.0 presumably

@lbajolet-hashicorp
Copy link
Contributor

I see there's a TODO list on the description of the PR, is it planned to check all three before merging it, or is it for follow-up issues?

@nywilken
Copy link
Contributor Author

I see there's a TODO list on the description of the PR, is it planned to check all three before merging it, or is it for follow-up issues?

Yeah, I plan to address these todos within this PR. I need to validate functionality and ensure docs reflect the correct plugin loading order. Thanks for the initial reivew.

@nywilken nywilken force-pushed the nywilken/plugin-scanning branch 2 times, most recently from bd28933 to b746f50 Compare May 26, 2023 20:00
Packer will try to discover installed plugins in all of the directories
defined by packer.KnowPluginFolders. In a previous release logic was
added to scan nested directories in order to load plugins installed by
`packer plugins install`. This change resulted in a nested directory
scan for each folder within the KnownPluginFolders slice.

This change reduces the nested directory scan to only the directories
where plugins would have been installed using `packer plugins install`
* PACKER_PLUGIN_PATH takes precedence over all
@nywilken nywilken added backport/1.9.x Backport PR changes to `release/1.9.x` bug enhancement and removed breaking-change enhancement labels Jun 22, 2023
@nywilken
Copy link
Contributor Author

Merging to make available in the latest nightly.

@github-actions
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/1.9.x Backport PR changes to `release/1.9.x` bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

packer tries to fork/exec plugin SHA256SUM file packer 1.8.6-1 recursively stats all files from working dir
2 participants