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

fixes #19254 - check Puppet version is supported by modules #213

Merged
merged 1 commit into from Jun 28, 2017

Conversation

domcleal
Copy link
Contributor

Using the Puppet version requirements from metadata.json in the
available modules, the running Puppet version is validated to ensure it
is compatible. The user can skip this with --skip-puppet-version-check
if they wish.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Generally it looks correct.

next unless metadata['requirements'] && metadata['requirements'].is_a?(Array)

metadata['requirements'].select { |req| req['name'] == 'puppet' }.each do |req|
checks << versioncmp(metadata['name'], req['version_requirement'])
Copy link
Member

Choose a reason for hiding this comment

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

Should this block als check if name and version_requirement are present or should it assume the metadata is valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've added a check for version_requirement into the select on the previous line, so this block will only run if both are set.

Using the Puppet version requirements from metadata.json in the
available modules, the running Puppet version is validated to ensure it
is compatible. The user can skip this with `--skip-puppet-version-check`
if they wish.
@ares
Copy link
Member

ares commented Apr 21, 2017

The PR looks good and works fine. This causes problems with foreman-installer running on puppet 3.8.6 and puppet-extlib 1.1.0 which requires 3.8.7+. I know there's a big group of users with this combination so I want to first make sure there's some better solution than asking them to run the installer with --skip-puppet-version-check. If you don't mind I'd do the release with what we have so far and keep this unmerged before it's solved.

@domcleal
Copy link
Contributor Author

domcleal commented Apr 21, 2017

Thanks, I wasn't aware of that incompatibility. Foreman installer 1.16.x will not support Puppet 3 at all, so it shouldn't matter that much - it would only be a problem with the modules shipped in 1.15 or older.

If something similar happened in the future, we would now be a little more aware of it through testing and could either correct the module metadata, or ship a version of the module that does claim to be compatible. Otherwise yes, a user would have to run with the skip argument, but I wouldn't expect that to be the norm.

Edit: no problem if you wish to release what's currently on master, and leave this unmerged.

@ares
Copy link
Member

ares commented Apr 25, 2017

Makes sense, I wanted to do a fix release compatible with 1.15

I think the best will be to bump the version to 2.1.0 once this is merged so if someone will require more fixes to be used with 1.15 we can always continue with 2.0 branch.

@ares ares added the 2.1 label Apr 25, 2017
@ares
Copy link
Member

ares commented Jun 28, 2017

Thanks @domcleal, merging.

@ares ares merged commit c55f9e4 into theforeman:master Jun 28, 2017
@ares ares added 2.1 and removed 2.1 labels Sep 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants