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
Fixes #28308 - ignore prerelease tags in dependencies #7188
Conversation
Issues: #28308 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but I believe we want oposite of what is implemented, right?
Now we can specify plugin: 2.1.0.rc2 dependency, and 2.1.0 version of the plugin would go, but I believe you are describing the oposite case in the comment.
lib/seed_helper.rb
Outdated
@@ -169,7 +169,7 @@ def test_template_requirements(template_name, requirements) | |||
if plugin.nil? | |||
logger.info("Template #{template_name} requires plugin #{r['plugin']}, skipping import.") | |||
return false | |||
elsif r['version'] && (Gem::Version.new(plugin.version) < Gem::Version.new(r['version'])) | |||
elsif r['version'] && (Gem::Version.new(plugin.version) < Gem::Version.new(r['version']).release) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we want the opposite, ritght?
test/unit/seed_helper_test.rb
Outdated
requirements = { | ||
'require' => [{ | ||
'plugin' => 'some_plugin', | ||
'version' => '2.0.1.rc2', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'version' => '2.0.1.rc2', | |
'version' => '2.0.1', |
test/unit/seed_helper_test.rb
Outdated
'version' => '2.0.1.rc2', | ||
}], | ||
} | ||
Foreman::Plugin.expects(:find).with('some_plugin').returns(mock(:version => '2.0.1')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Foreman::Plugin.expects(:find).with('some_plugin').returns(mock(:version => '2.0.1')) | |
Foreman::Plugin.expects(:find).with('some_plugin').returns(mock(:version => '2.0.1.rc2')) |
Templates with requirements specifying plugins that contains .rc in their versions are skipped. E.g. a template needs katello 3.14.0 but 3.14.0.rc1 s installed. The comparison does not work and template is ignored. We should ignore the .rc tag as 3.14.0.rc1 has all functionality already and should be only receiving bug fixes. That makes tester life easier, they don't have to hack templates in order to seed them.
You're of course right, I repushed the right version now. Seems it was a long day yesterday :-) |
@@ -169,7 +169,7 @@ def test_template_requirements(template_name, requirements) | |||
if plugin.nil? | |||
logger.info("Template #{template_name} requires plugin #{r['plugin']}, skipping import.") | |||
return false | |||
elsif r['version'] && (Gem::Version.new(plugin.version) < Gem::Version.new(r['version'])) | |||
elsif r['version'] && (Gem::Version.new(plugin.version).release < Gem::Version.new(r['version'])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we care about specifying a prerelease version in the template? because if we do this might lead to unexpected result:
[11] pry(main)> Gem::Version.new('1.2.0.rc1').release < Gem::Version.new('1.2.0.rc2')
=> false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we do. I don't see a template to require a RC release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, let's keep it simple and only keep versioning on GA versions, technically we may want to depend on something we added only in rc, but in real world, we care about Katello 3.14, not any of it's RC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works for me
1.24 - f98ccd2 |
Templates with requirements specifying plugins that contains .rc in their
versions are skipped. E.g. a template needs katello 3.14.0 but 3.14.0.rc1
s installed. The comparison does not work and template is ignored.
We should ignore the .rc tag as 3.14.0.rc1 has all functionality already
and should be only receiving bug fixes. That makes tester life easier,
they don't have to hack templates in order to seed them.