Skip to content

(MODULES-3354) Use 1.8.7 hash in validate_email_address function #606

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

Merged
merged 1 commit into from
May 11, 2016

Conversation

stbenjam
Copy link
Contributor

Otherwise you get this on 1.8.7:

/usr/lib/ruby/site_ruby/1.8/rubygems/custom_require.rb:31:in `gem_original_require': /usr/share/foreman-installer/modules/stdlib/lib/puppet/parser/functions/validate_email_address.rb:2: syntax error, unexpected ':', expecting ')' (SyntaxError)
newfunction(:validate_email_address, doc: <<-ENDHEREDOC

@stbenjam
Copy link
Contributor Author

There was another one recently in #598, rubocop can enforce a hash style if you wanted to do so on ruby code in this repo.

@bastelfreak
Copy link
Collaborator

Hi,
Ruby1.8.7 is EOL since a very long time. There are several open security issues that won't be fixed. Have you considered updating to a newer version? I know that RedHat still "supports" Ruby1.8.7 on RHEL6. But RHEL7 is out since years (and also ships an outdated version...), but still has a way newer version. Also puppet brings the AIO packages with a bundled Ruby version which even works on RHEL5 I think. A thrid option would be to package your own ruby, even this is easy.


I'm against this patch. Ruby1.8 restricts us from a developer side and is totally unresponsible from an Ops side. There are enough alternative solutions to get a newer ruby. Feel free to join #voxpupuli or #puppet on freenode and ping me there (or anybody else). We're happy to help you migrating to a newer ruby/OS/Puppet version.

@stbenjam
Copy link
Contributor Author

stbenjam commented May 11, 2016

I'm aware of all of this, but I don't need any help. Thanks for the offer.

Foreman and Katello project installers are puppet based via Kafo, and support system ruby on EL6 (the applications run inside much newer Ruby through software collections), and we utilize this module.

First, the hash style in stdlib is clearly hash rockets. If that changes, it should be changed it everywhere and be consistent instead. As it stands, the module is otherwise compatible with 1.8.7.

Second, this was introduced in a patch minor release and that's not an acceptable time to introduce such a change from a semver perspective.

I don't care if stdlib wants to drop 1.8.7 support, but it should be done appropriately.

@bastelfreak
Copy link
Collaborator

It isn't clearly defined in semver if increasing the minimum required version number of a dependency requires a major bump. We had a huge discussion about that in guard/listen#374, but I share you opinion. @hunner what do you think about a major bump in stdlib with official drop of ruby 1.8.7 and migration to the new Style/HashSyntax everywhere? I could provide a PR if anybody wants it.

@stbenjam
Copy link
Contributor Author

It isn't clearly defined in semver if increasing the minimum required version number of a dependency requires a major bump.

That's fair enough, but it doesn't really appear the drop in 1.8.7 support was intentional, the doc hash was just overlooked. If it doesn't cost much to continue it working for 1.8.7, it'd be great if this was accepted like #548 was.

I'm very much looking forward to forgetting 1.8.7 forever, but unfortunately that's still some time away. :-\

@hunner
Copy link
Contributor

hunner commented May 11, 2016

@stbenjam Please be aware that we don't test on 1.8, code for 1.8, or claim compatibility/support for 1.8, which means any new module release could break on 1.8 again in the future.

stdlib's compatibility is currently based on puppet versions in metadata.json, not ruby versions. It currently claims compatibility with puppet 2.7.20 as a minimum, and 2.7.20 is ruby 1.9 compatible so doesn't imply ruby 1.8. I know this isn't very straight-forward, but I hope you can understand!

@bastelfreak I have definitely wanted to make a stdlib 5 release that drops puppet 2.7 but stdlib should really be the last one out the door as it is depended on by so many other modules.

@hunner hunner merged commit 7a008a7 into puppetlabs:master May 11, 2016
@stbenjam
Copy link
Contributor Author

stbenjam commented May 11, 2016

stdlib's compatibility is currently based on puppet versions in metadata.json, not ruby versions. It currently claims compatibility with puppet 2.7.20 as a minimum, and 2.7.20 is ruby 1.9 compatible so doesn't imply ruby 1.8. I know this isn't very straight-forward, but I hope you can understand!

2.7.20 is also 1.8.7 compatible (and even 1.8.5, I believe haha), so one could argue it should support all the platforms that 2.7.20 runs on. Although, I realize these are both dead platforms for quite some time so one could also make different arguments.

It almost seems like metadata.json could use an understanding of ruby versions, as many modules ship ruby code in lib/.

Anyway, thanks! This keeps our ruby 1.8.7 party going on for a little while longer.

@stbenjam stbenjam deleted the MODULES-3354 branch May 11, 2016 23:57
@hunner
Copy link
Contributor

hunner commented May 12, 2016

Yeah, metadata.json specifying ruby versions sounds like a reasonable feature.

Honestly the decision to support a ruby of 2.7.20 instead of all rubies is that we just don't have the internal bandwidth to handle the number of issues we were running into before we started dropping it. I'll bring up keeping stdlib for 1.8.7 as it should be "the last one out the door" as mentioned above, at least until 2.7.20 is out.

hunner referenced this pull request Apr 5, 2017
* (#FM-6068) allow file encoding to be specified

Add a new parameter `encoding` to allow non UTF-8 files to specify a file encoding.  This prevents receiving the error message "invalid byte sequence in UTF-8" when special characters that are not UTF-8 encoded appear in the input stream, such as the copyright symbol.

* (#FM-6068) allow file encoding to be specified
Added docs and tests as requested
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants