(PUP-7738) Ensure that Puppet can be used with semantic_puppet gem.#6030
Conversation
|
CLA signed by all contributors. |
| # @api private | ||
| def self.parse_range(range, strict) | ||
| @parse_range_method ||= SemanticPuppet::VersionRange.method(:parse) | ||
| if @parse_range_method.arity == 1 |
There was a problem hiding this comment.
this is the coolest block of ruby I read this month. I didn't know that ruby is able to tell you the amount of arguments.
Thanks for quickly providing such an awesome fix!
lib/puppet/module.rb
Outdated
| if strict | ||
| if @semver_gem_version.major < 1 | ||
| Puppet.warn_once('strict_version_ranges', 'version_range_cannot_be_strict', | ||
| _('VersionRanges will never be strict when using non-vendored SemanticPuppet gem, version %{version}') % { version: @semver_gem_version} ) |
There was a problem hiding this comment.
It looks like there are a few places where strict_semver is not getting passed through, so it defaults to true and triggers the warning when --strict_semver is not specified. For example, given a Gemfile:
source "https://rubygems.org"
gem "puppet", :path => '/Users/josh/work/puppet'
gem "semantic_puppet", "~> 0.1.4"
Install gems, including old semantic_puppet:
$ bundle install --path .bundle
Fetching gem metadata from https://rubygems.org/........
Fetching version metadata from https://rubygems.org/.
Resolving dependencies...
Installing CFPropertyList 2.2.8
Installing fast_gettext 1.1.0
Installing locale 2.1.2
Installing text 1.3.1
Installing hiera 3.4.0
Installing hocon 1.2.5
Installing net-ssh 4.1.0
Using bundler 1.14.6
Installing facter 2.4.6 (universal-darwin)
Installing gettext 3.2.3
Installing gettext-setup 0.25
Using puppet 5.0.0 from source at `/Users/josh/work/puppet`
Installing semantic_puppet 0.1.4
Bundle complete! 2 Gemfile dependencies, 13 gems now installed.
Bundled gems are installed into ./.bundle.
This is not expected:
$ rm -rf ~/.puppetlabs/etc/code/modules
$ bundle exec puppet module install puppetlabs-apache
Notice: Preparing to install into /Users/josh/.puppetlabs/etc/code/modules ...
Notice: Created target directory /Users/josh/.puppetlabs/etc/code/modules
Notice: Downloading from https://forgeapi.puppet.com ...
Warning: VersionRanges will never be strict when using non-vendored SemanticPuppet gem, version 0.1.4
(file & line not available)
Notice: Installing -- do not interrupt ...
/Users/josh/.puppetlabs/etc/code/modules
└─┬ puppetlabs-apache (v1.11.0)
├── puppetlabs-concat (v2.2.1)
└── puppetlabs-stdlib (v4.17.1)
I would only expect the warning when using puppet module install --strict-semver. It looks like Puppet::Forge calls:
ModuleRelease.new(self, release)
which defaults strict_semver to true triggering the behavior.
lib/puppet/module.rb
Outdated
| else | ||
| if @semver_gem_version.major >= 1 | ||
| Puppet.warn_once('strict_version_ranges', 'version_range_always_strict', | ||
| _('VersionRanges will always be strict when using non-vendored SemanticPuppet gem, version %{version}') % { version: @semver_gem_version} ) |
There was a problem hiding this comment.
To "resolve" this warning a user would be forced to run with --strict_semver, even though they may not be anything wrong with the versions. That seems overly invasive for little gain.
There was a problem hiding this comment.
I partly agree. But please consider that:
- the warning is only output once
- the warning is only output when they are using the external gem
- if they indeed do something wrong, and run into problems, there's no way of telling why.
|
@thallgren looks like valid travis/appveyor failures. |
This commit adds the method `Puppet::Module#parse_range` which in turn calls `SemanticPuppet::VersionRange#parse` and ensures that all calls from `Puppet::Module` and `Puppet::ModuleTool` uses that method instead of calling directly.
This commit adds an arity check to be able to call the method `SemanticPuppet::VersionRange#parse` with one or two arguments from the `Puppet::Module#parse_range` method. When the arity is found to be just 1, a check is made if the gem version of the semantic_puppet gem is < 1.0.0, in which case it is assumed that the parse method is incapable of creating a range that adheres to strict semantics. A warning is logged if a request for a strict range is made. Conversely, if the gem version is >=1.0.0, it is assumed that strict semantics will be honored and a warning is issued if a request for a non-strict range is made.
Before this commit, a created `Puppet::Forge` instance would not know the setting of `strict_semver`. This made parts of the dependency resolution use strict semantics. This commit ensures that the setting is known to `Puppet::Forge` and propagated to the created `ModuleRelease` instances when processing a dependency tree.
It appears that the while the unit test for the `search` face that tested if the option `:module_repository` was accepted, it did not check if it was also propagated to the `Puppet::Forge` instance. It wasn't, and as a consequence, the test failed when the default repository was propagated in an attempt to simulate the default argument assignment when passing no arguments to `Puppet::Forge#new`. This commit therefore fixes two things: 1. It ensures that the `:module_repository` option is propagated when the `Puppet::Forge` is instantiated. 2. It updates the test to check that the correct repository is propagated when the instance is created.
thallgren
force-pushed
the
issue/pup-7738/check-range-parse-arity
branch
from
690a6c3 to
89c3322
Compare
There was a problem hiding this comment.
This still isn't working as I would expect. If I'm using semantic_puppet 1.0.1 as an external gem, then I always receive a warning even when using --strict-semver. So I'm getting a warning about something I can't do anything to fix. And I don't think we want to encourage module developers to remove semantic_puppet from their Gemfiles as we want to unvendor from Puppet.
$ rm -rf ~/.puppetlabs/etc/code/modules
$ bx puppet module install puppetlabs-apache
Notice: Preparing to install into /Users/josh/.puppetlabs/etc/code/modules ...
Notice: Created target directory /Users/josh/.puppetlabs/etc/code/modules
Notice: Downloading from https://forgeapi.puppet.com ...
Warning: VersionRanges will always be strict when using non-vendored SemanticPuppet gem, version 1.0.1
(file & line not available)
Notice: Installing -- do not interrupt ...
/Users/josh/.puppetlabs/etc/code/modules
└─┬ puppetlabs-apache (v1.11.0)
├── puppetlabs-concat (v2.2.1)
└── puppetlabs-stdlib (v4.17.1)
$ rm -rf ~/.puppetlabs/etc/code/modules
$ bx puppet module install puppetlabs-apache --strict-semver
Notice: Preparing to install into /Users/josh/.puppetlabs/etc/code/modules ...
Notice: Created target directory /Users/josh/.puppetlabs/etc/code/modules
Notice: Downloading from https://forgeapi.puppet.com ...
Warning: VersionRanges will always be strict when using non-vendored SemanticPuppet gem, version 1.0.1
(file & line not available)
Notice: Installing -- do not interrupt ...
/Users/josh/.puppetlabs/etc/code/modules
└─┬ puppetlabs-apache (v1.11.0)
├── puppetlabs-concat (v2.2.1)
└── puppetlabs-stdlib (v4.17.1)
where my Gemfile contains:
source "https://rubygems.org"
gem "puppet", :git => 'https://github.com/thallgren/puppet.git', :branch => 'issue/pup-7738/check-range-parse-arity'
gem "semantic_puppet", "~> 1.0.1"Prior to this commit, the method `Puppet::ModuleTool#set_default_options` would set the `:strict_semver` option to `false` even though it had already been assigned a value. This commit ensures that the given value is retained and adds tests to assert that.
|
So this seems to be working now, but I still have questions. If If Also if |
|
How about if we drop the |
|
The problem is that we cannot see if there's something wrong with a I think the warning should stay in, because the patched |
|
In the scenario above, puppet is emitting a warning even though the apache module is using semantically correct version ranges. That makes no sense. A warning indicates something has gone wrong, but in this case, nothing is wrong. The apache module is correct, and editing |
|
In your scenario, the resolution above will differ if a pre-release comes into existence for stdlib or concat. If the user uses an external 1.0.1, those pre-releases will not be selected. Perhaps that will never happen for this particular module? In any case, I'm reluctant to create a design that makes that assumption for all modules. That said, I do understand your objection to using a warning. Perhaps the message should be a Notice (once)? |
|
Please change it to a Notice or Info (once). |
In some cases, it's desirable to make the user aware of potential issues that may or may not cause a problem. This commit makes it possible to use the `warn_once` method to log such problems with a different log-level than `:warning`.
This commit changes the messages that are logged when the setting of `strict_semver` cannot be honored by the `semantic_puppet` implementation so that they are logged using `:notice` instead of `:warning`.
Adds ability to pass `:default` instead of `nil` for the file and line arguments passed to `#warn_once`. When passing :default for both arguments, the output "file & line not available" is suppressed.
|
From module that depends on |
|
From puppet (so no external semantic_puppet gem): |
4 participants
This commit adds an arity check to be able to call the method
SemanticPuppet::VersionRange#parsewith one or two arguments from thePuppet::Module#parse_rangemethod.When the arity is found to be just 1, a check is made if the gem version
of the semantic_puppet gem is < 1.0.0, in which case it is assumed that
the parse method is incapable of creating a range that adheres to strict
semantics. A warning is logged if a request for a strict range is made.
Conversely, if the gem version is >=1.0.0, it is assumed that strict
semantics will be honored and a warning is issued if a request for a
non-strict range is made.