search

voxpupuli / puppet-network

Types and providers to manage network interfaces
https://forge.puppet.com/puppet/network
Apache License 2.0
66 stars 108 forks source link

Fix facts to be Ruby 1.8 compatible #184

Closed vholer closed 8 years ago

vholer commented 8 years ago

Currently network facts work with latest Facter 3.4, but fails with older Facter 2.4 with invalid syntax:

/var/lib/puppet/lib/facter/network.rb:9: syntax error, unexpected ':', expecting kEND
  confine kernel: :linux
                 ^
/var/lib/puppet/lib/facter/network.rb:24: syntax error, unexpected ':', expecting kEND
  confine kernel: :linux
                 ^
/var/lib/puppet/lib/facter/network.rb:45: syntax error, unexpected ':', expecting kEND
  confine kernel: :linux
                 ^

Current documentation still proposes using confine key => value format https://docs.puppet.com/facter/3.4/custom_facts.html#confining-facts so I feel it's still better to stick with that syntax.

alexjfisher commented 8 years ago

@vholer I don't think it's the version of facter that's causing you problems, but rather your ruby version.

My guess is you're using RHEL6 (or variant such as CentOS 6)?

This module doesn't support ruby 1.8 any more. Puppet 4 (which includes facter 3), ships with its own ruby (2.1? IIRC) instead of relying on the system ruby.

vholer commented 8 years ago

@alexjfisher Yeah, you are absolute right. It's failing on RHEL 6 running older Ruby with older Puppet 3.8 and older Facter and I got it working on latest Puppet bundle from PC1.

OK and is there any other reason here than "we want you to stop using old Puppets" why is there a backward incompatible syntax? This approach only results in forking your repo, trivial fixing and use own fork as long as possible. No real benefit for you. I talk only about this case of "broken" facts, not in general.

Thanks for considering this pull request.

ffrank commented 8 years ago

I suppose if this is all that's needed to get better compatibility, we can go for it(?)

vholer commented 8 years ago

It's up to you. I fully understand if you want to move forward.

alexjfisher commented 8 years ago

@ffrank Generally speaking, no, but for facts, exceptions have been made in the past. eg https://github.com/voxpupuli/puppet-archive/commit/f38b8843dc682a39a9acaa659e18f9f139deaefd

Vox Pupuli don't support ruby 1.8 in any of their modules. In the case of facts, they get run on all machines in your environment even if you're not otherwise using the module on that host.

The typical case is that you might have some legacy EL6 (ruby 1.8) boxes and newer EL7 (ruby 2 IIRC) boxes in your environment. You want to use puppet-network (for example) to configure your EL7 hosts, but you don't want facter blowing up on your EL6 hosts.

alexjfisher commented 8 years ago

@vholer If you're happy with the limitation that only the facts will be fixed for ruby 1.8 and the types will still not be usable on your old boxes, then could you update your PR to include # rubocop:disable Style/HashSyntax at the end of each line you've changed?

vholer commented 8 years ago

@alexjfisher I'm absolutely happy with this solution. PR fixed. Thanks!

alexjfisher commented 8 years ago

@vholer If you could rebase against master, I expect the build to go green again.

vholer commented 8 years ago

Hope it's OK now. Thanks.

ffrank commented 8 years ago

One last thing - could you squash the merge commit and force push once more? I'll merge it as soon as it's cleaned up. Let me know if you need help with that.

vholer commented 8 years ago

@ffrank Yes, please. It would be better if you can guide me in more detail so that I don't break it to you. Actually I can see following history:

$ git log -4 --pretty=format:"%h %an %s"
e02d1fe Vlastimil Holer Merge remote-tracking branch 'upstream/master'
9c517df Alex Fisher Merge pull request #186 from dhoppe/modulesync
092326d Dennis Hoppe Update based on voxpupuli/modulesync_config 0.12.7
d20f30a Vlastimil Holer Make fact confinement ruby 1.8 compatible

Thanks.

alexjfisher commented 8 years ago

@vholer Ah, you merged instead of doing a rebase. https://www.atlassian.com/git/tutorials/merging-vs-rebasing

Don't worry, I can pick up your commit and rebase it for you.

alexjfisher commented 8 years ago

@vholer Rebasing was always going to be tricky because you made your commit on your fork's master branch. Ideally, you only ever want your master branch to contain commits that have come from upstream master. For developing changes you're better off using a local topic branch. Rebasing might then look a bit like ...

git remote add upstream https://github.com/voxpupuli/puppet-network.git
git checkout master
git fetch upstream
git merge upstream/master
git push # optional.  Updates your github fork with latest master changes
git checkout feature_branch_you_need_to_rebase
git rebase -i master
git push --force # Force-push your modified branch back to github.  This will update your PR automatically.

Hope this helps.

If you do ever get stuck, feel free to drop by #voxpupuli on IRC.

alexjfisher commented 8 years ago

Merged in https://github.com/voxpupuli/puppet-network/pull/187