packer: support loading pre-release plugins #12828
Merged
+567
−975
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lbajolet-hashicorp
force-pushed
the
support_prerelease_loading
branch
2 times, most recently
from
a36b427
to
6bdb8ff
Compare
nywilken
reviewed
Feb 23, 2024
nywilken
reviewed
Feb 23, 2024
nywilken
reviewed
Feb 23, 2024
There was a problem hiding this comment.
Overall the changes look good. The failed tests are due to the tests plugins github.com/sylviamoss/comment having mixed versions that fail the newly added plugin version validation check (version filename must equal the version in the describe command). I think we should move to a trusted plugin for these test (e.g packer-plugins-hashicups).
I left a few suggestions but I think the approach is good to go.
lbajolet-hashicorp
force-pushed
the
hardened_plugin_loading
branch
from
bb563f7
to
e1b2669
Compare
Since we're removing the alternative plugin installation directories in favour of only supporting installing them in the PACKER_PLUGIN_PATH directory, we only return one directory when getting the known plugin directories.
Since we'll be only accepting plugins installed locally along with a shasum, and those that adopt the convention we introduced with required_plugins, we remove support for plugins that only have a packer-plugin-.* type name.
Since we'll only look in the plugin directory, and not from multiple sources now, for installing/listing plugins, we can simplify the structures which used to accept multiple directories so they only accept one.
Since the plugin directory should now be unique instead of a list, we can use it directly from the receiver's structure instead of as a parameter to the function.
The comments weren't capitalised correctly, and the directory example included the `packer-plugin-' prefix, which should not be the case in reality.
The plugin installation list should be sorted according to a name first, then version second basis. Right now, we only rely on the glob to add plugin installs to this list, making the order unreliable since the lexicographical order is not the order in which we want to see the same plugin ordered (e.g. v1.0.9 > 1.0.10). To fix this, we implement a logic for sorting a list of installations that does what's described above with more accuracy.
Right now we had two paths for discovering installed plugins, i.e. through plugin-getter's `ListInstallations' function, or through the `Discover' call, which relied on a glob to list the installations. This was required since we allowed plugins to be installed in multiple locations, and with different constraints. Now that we force a certain convention, we can consolidate the logic into ListInstallations, and rely on that logic in `Discover' to load a plugin into the PluginConfig for the current Packer run.
When a plugin is loaded from Packer, we now check that the version it reports matches what the name implies. In case there's a mismatch, we log, and reject the binary.
Since the plugins remove subcommand now only loads valid plugins, it cannot run on mock data anymore, and the logic for creating a valid plugin hierarchy is not present in this repository, but does in another, so we move those tests from here to that repository in order to continue testing them.
When Packer orders the plugins by their version, we'd assumed it was ordered from highest to lowest. However, this was the case because our implementation of `Less' was actually returning `>=', which is not what it was supposed to do. This commit therefore fixes the implementation of `Less' to do what it is documented to do, and ensures the behaviour is correct through additional testing. Changing this requires some changes to the loading process as well because of the aforementioned assumption with regards to ordering.
When a pre-release version of a plugin is locally installed, it may or may not be loaded depending on the constraints expressed in the template being executed. If the template contains constraints for loading the plugin, it would be ignored, while if that wasn't present, it would be loaded. This is inconsistent, and deserves to be addressed, which is what this commit does. With this change, plugin pre-releases are now loaded, provided the version reported matches the constraints, independently from its pre-release suffix `-dev'. Also, if a release with the same version is also installed alongside the pre-release version of a plugin, it will have precedence over the pre-release.
When Packer is loaded, we used to perform plugin discovery. This was done for every call to Packer, including when it is executed as a plugin, arguably against what the comments document. Doing this as early in the loading process makes it harder to change this behaviour, as we'd need to introduce flags aside from the rest, and handle them manually, which is not optimal. Therefore, we change this: now when Packer starts executing, it will not attempt to discover installed plugins anymore, and instead will only try to load them when a configuration has been parsed, and is being used to perform actions (typically build/validate).
lbajolet-hashicorp
force-pushed
the
support_prerelease_loading
branch
from
6bdb8ff
to
2a073a8
Compare
lbajolet-hashicorp
changed the base branch from
hardened_plugin_loading
to
main
lbajolet-hashicorp
force-pushed
the
support_prerelease_loading
branch
3 times, most recently
from
30df3a9
to
3de3e5a
Compare
Since we now support loading pre-releases, we also want Packer to be able to ignore them by user demand, so we put in place the infrastructure to modulate this.
Since we added the capability for the plugin discovery to ignore installed pre-releases of plugins, we give that capacity to the validate and build commands, through a new command-line flag: release-only.
Since we now support loading pre-releases with Packer subcommands, we relax the contraints we had placed on the `--path' option, so it will accept any `-dev' binary, in addition to final releases.
In order to test the Packer subcommands, some tests rely on a plugin: github.com/sylviamoss/comment. This plugin is outdated compared to our SDK, and some of the binaries releases don't match what is expected by Packer, i.e. v1.0.0 vs. v0.2.8. To fix that, we migrate to use the hashicups plugin, which is our demo plugin for Packer, and is actually maintained, and up-to-date, which will make those tests more reliable moving forward.
While migrating to a unified approach for loading plugins, the API major and minor versions were not added to the constraints for discovering plugins from the environment, leading to Packer potentially considering plugins that are not compatible with itself. This should not be possible, but was due to this omission, which we fix with this commit.
lbajolet-hashicorp
force-pushed
the
support_prerelease_loading
branch
from
3de3e5a
to
bf1895d
Compare
|
Apologies you were waiting on me for the approval 🚢 |
nywilken
reviewed
Mar 1, 2024
nywilken
approved these changes
Mar 1, 2024
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a pre-release version of a plugin is locally installed, it may or may not be loaded depending on the constraints expressed in the template being executed.
If the template contains constraints for loading the plugin, it would be ignored, while if that wasn't present, it would be loaded.
This is inconsistent, and deserves to be addressed, which is what this commit does.
With this change, plugin pre-releases are now loaded, provided the version reported matches the constraints, independently from its pre-release suffix `-dev'.
Also, if a release with the same version is also installed alongside the pre-release version of a plugin, it will have precedence over the pre-release.
Note: based on top of #12787 for readability