Add boolean to control EPEL #181

lukebigum · 2017-07-11T16:24:22Z

Add boolean to explicitly control whether Class[python] tries to manage EPEL or not - useful for environments that mirror EPEL internally.

This directly conflicts with #176 - but my commit is backwards compatible as EPEL will continue to be managed by default.

Note the Rubocop failure is on spec/spec_helper_acceptance.rb which is probably managed by Voxpupuli's Module Sync, so while I could make the Rubocop warning go away, my change will be overwritten by any msync from upstream :-)

alexjfisher · 2017-07-11T21:18:54Z

manifests/init.pp

+# [*python_use_epel*]
+#   (bool) Whether the Python class will use attempt to manage EPEL or not.
+#   Defaults to true ($::puppetboard::params::python_use_epel)
+#


The default for fedora users was actually false though. https://github.com/stankevich/puppet-python/blob/718e9bc482f72559d9035ec2b8b651be01984128/manifests/params.pp#L25 So this would change the default behaviour for them.

I'm not sure we want to be replicating the logic in that class in our own params.pp though... Dunno. Maybe someone else will have a suggestion.

alexjfisher · 2017-07-11T21:29:53Z

manifests/init.pp

@@ -289,6 +294,7 @@
    class { '::python':
      virtualenv => 'present',
      dev        => 'present',
+      use_epel   => $python_use_epel,


This might also be backwards incompatible for any users that have already worked around the issue by setting python::use_epel: false in their hiera?

I forgot how hiera handles this. If it's undef, won't it still use the hiera value?

Why is it an optional boolean vs. just a boolean again? And even if they had set python::use_epel: false earlier, would this actually generate an error if this setting were also false, or just be redundant?

Ah I see, to try to avoid changing existing behavior. Makes sense.

@ekohl Not sure. It's probably not too much of a big deal.

lukebigum · 2017-07-12T09:32:59Z

Both good points. Attempt no. 2 - Boolean is now a Variant and defaults to undef, so the internal behaviours won't change (ie: Fedora) and Hiera will still kick in if people are tuning Class[python] it that way.

FYI: this set of tests looks broken. /etc/apache2 on Red Hat...? Leaving it alone, I think it's working on Travis by magic (it fails when I test locally).

alexjfisher · 2017-07-12T09:40:50Z

manifests/init.pp

@@ -173,6 +177,7 @@
  $python_loglevel                                            = $::puppetboard::params::python_loglevel,
  $python_proxy                                               = $::puppetboard::params::python_proxy,
  $python_index                                               = $::puppetboard::params::python_index,
+  Variant[Boolean, Undef] $python_use_epel                    = undef,


It's probably more common to use Optional[Boolean] $var_name = undef but I think that's just a style thing.

lukebigum · 2017-07-12T11:07:58Z

IChanged to an Optional.

lukebigum · 2017-07-21T08:56:49Z

Anything else holding up this PR?

bastelfreak · 2017-07-21T13:17:28Z

Hi @lukebigum, could you also provide a fix for the rubocop issue?

lukebigum · 2017-07-22T12:36:19Z

Hi, the Rubocop issue is on spec/spec_helper_acceptance.rb, which I assumed was managed by Voxpupuli's Module Sync. I can fix it, but it might just get overwritten again.

alexjfisher · 2017-07-22T14:49:21Z

@lukebigum Good news is spec/spec_helper_acceptance.rb is not managed my modulesync. If you think you can fix it, that'd be much appreciated.

lukebigum · 2017-07-24T14:04:06Z

@alexjfisher I've looked, but now I think it's a false positive. The code style that rubocop-rspec is trying to catch is geared towards a very simple array loop in rspec, and this is not an Rspec test file (thus not possible to use rspec's all matcher here). I've disabled that test for that file.

tux-o-matic · 2017-07-27T11:56:11Z

@lukebigum, what's the point if EPEL is actually not needed?

lukebigum · 2017-07-28T10:32:43Z

@tux-o-matic I don't understand what you mean. Class[puppetboard] declares Class[python] in such a way that it pulls in Class[epel] by default. This patch changes Class[puppetboard] so that it declares Class[python] with the 'use_epel' parameter, allowing you to turn that off, if say, you don't want to Class[epel] in your catalog.

tux-o-matic · 2017-07-28T10:49:54Z

@lukebigum, as I discovered out in #166, the only reason to need EPEL is for virtualenv on EL 6. And to get virtualenv on EL 6 you could within this module:

Define the Yumrepo resource directly.
Install virtualenv as a Package resource with the 'pip' provider.
You have two built-in Python resource types to support that one OS instead of loading a third party module.

lukebigum · 2017-07-28T10:54:38Z

@tux-o-matic that's exactly what I don't want to do. Such an approach assumes that you can simply create Yumrepo resources and Pip packages in my environment and it would all work perfectly. That would work for most people with free access to the Internet, think... "behind giant corporate firewall".

I could do that, but again I'd need to wrap all of said functionality in a Boolean so I personally could turn it off.

tux-o-matic · 2017-07-31T06:54:26Z

@lukebigum The Yumrepo resource supports the proxy option, I've been using it. The pip provider can inherit proxy settings from the environment, can't recall how but I frequently use it on nodes behind a proxy.

lukebigum · 2017-07-31T09:58:52Z

@tux-o-matic, the ability to proxy may help some people, but that's still not the point. My EPEL repository is already on the box, put there with Pulp, and it works perfectly fine. I don't need to proxy anything, nor do I need this module to help me manage EPEL, which is the real issue. This module assumes too much. It is trying to be too helpful. Dare I say... It's like the Microsoft Word Paperclip :-)

There are many good examples of modules that by default try to by helpful and set up lots of for you, but still allow you to disable this functionality if you have the need to do it yourself. ajcrowe/supervisord is one good example.

I'm all for being helpful, but at least allow me to turn it off, or merge my PR that allows me to turn it off ;-)

tux-o-matic · 2017-07-31T15:08:46Z

I agree, it's trying to do too much by default.

lukebigum · 2017-09-01T16:01:10Z

Shameless bump. Both this PR and #166 are trying to make this module less tightly coupled. Anyone want to merge or provide additional feedback on either?

tux-o-matic · 2017-09-04T06:27:26Z

@lukebigum, you can take your PR further. I made a whole new module to install Puppetboard so I'm no longer using this module .

wyardley · 2017-09-17T18:41:49Z

@alexjfisher anything remaining that needs to be changed for this one?

alexjfisher · 2017-09-18T11:57:50Z

manifests/params.pp

@@ -54,6 +54,7 @@
  $python_loglevel = 'info'
  $python_proxy = false
  $python_index = false
+  $python_use_epel = true


What does this do? You are defaulting to undef in init.pp not $::puppetboard::params::python_use_epel

I think this just needs coming out.

lukebigum · 2017-09-18T12:17:30Z

Good spot, left over from a previous change. Removed the unused variable and also fixed a rubocop merge conflict.

ekohl · 2017-10-19T14:14:38Z

It looks like you merged master. It would help reviewing if you rebased on master instead.

lukebigum · 2017-10-19T14:59:34Z

Fixed my poor merge, please let me know any other feedback.

wyardley · 2017-10-19T15:17:37Z

.rubocop.yml

@@ -527,6 +528,10 @@ RSpec/NestedGroups:
 Layout/IndentHeredoc:
  Enabled: False

+Rspec/IteratedExpectation:


I don't think this is necessary now, .rubocop.yml is managed by modulesync, but current master doesn't have any rubocop warnings, so as long as this PR didn't change spec_helper_acceptance, shouldn't be necessary.

For some reason, I don't see this when I check out the PR locally.

…ge EPEL or not - useful for environments that mirror EPEL internally.

lukebigum · 2017-10-19T15:34:52Z

You are right, I've removed the rubocop change and it still passes.

alexjfisher · 2017-10-24T10:34:36Z

Thanks @lukebigum!

alexjfisher reviewed Jul 11, 2017

View reviewed changes

lukebigum force-pushed the master branch 5 times, most recently from 7427cb3 to 9dc632b Compare July 12, 2017 09:24

lukebigum force-pushed the master branch from 9dc632b to 2a67f36 Compare July 12, 2017 09:35

alexjfisher reviewed Jul 12, 2017

View reviewed changes

lukebigum force-pushed the master branch from 2a67f36 to 44d0f65 Compare July 12, 2017 11:02

lukebigum force-pushed the master branch 6 times, most recently from 5fc13fb to 7946ff8 Compare July 24, 2017 13:55

wyardley added the enhancement New feature or request label Sep 17, 2017

wyardley mentioned this pull request Sep 17, 2017

#166: Removed direct dependencies on EPEL module #176

Closed

alexjfisher requested changes Sep 18, 2017

View reviewed changes

lukebigum force-pushed the master branch 2 times, most recently from 1bf672f to f59ad70 Compare September 18, 2017 12:07

wyardley added the needs-rebase label Sep 26, 2017

lukebigum closed this Oct 19, 2017

lukebigum force-pushed the master branch from 600ff47 to 6904d70 Compare October 19, 2017 14:33

lukebigum reopened this Oct 19, 2017

lukebigum closed this Oct 19, 2017

lukebigum force-pushed the master branch from f36149e to 6904d70 Compare October 19, 2017 14:40

lukebigum reopened this Oct 19, 2017

wyardley removed the needs-rebase label Oct 19, 2017

wyardley reviewed Oct 19, 2017

View reviewed changes

Add boolean to explicitly control whether Class[python] tries to mana…

8e48a0d

…ge EPEL or not - useful for environments that mirror EPEL internally.

lukebigum force-pushed the master branch from f4e6dc7 to 8e48a0d Compare October 19, 2017 15:23

alexjfisher approved these changes Oct 24, 2017

View reviewed changes

ekohl approved these changes Oct 24, 2017

View reviewed changes

alexjfisher merged commit 1181bd4 into voxpupuli:master Oct 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add boolean to control EPEL #181

Add boolean to control EPEL #181

lukebigum commented Jul 11, 2017

alexjfisher Jul 11, 2017

alexjfisher Jul 11, 2017

ekohl Oct 19, 2017

wyardley Oct 19, 2017

wyardley Oct 19, 2017

alexjfisher Oct 24, 2017

lukebigum commented Jul 12, 2017

alexjfisher Jul 12, 2017

lukebigum commented Jul 12, 2017

lukebigum commented Jul 21, 2017

bastelfreak commented Jul 21, 2017

lukebigum commented Jul 22, 2017

alexjfisher commented Jul 22, 2017

lukebigum commented Jul 24, 2017

tux-o-matic commented Jul 27, 2017

lukebigum commented Jul 28, 2017

tux-o-matic commented Jul 28, 2017

lukebigum commented Jul 28, 2017

tux-o-matic commented Jul 31, 2017 •

edited

Loading

lukebigum commented Jul 31, 2017

tux-o-matic commented Jul 31, 2017

lukebigum commented Sep 1, 2017

tux-o-matic commented Sep 4, 2017

wyardley commented Sep 17, 2017

alexjfisher Sep 18, 2017

alexjfisher Sep 18, 2017

lukebigum commented Sep 18, 2017

ekohl commented Oct 19, 2017

lukebigum commented Oct 19, 2017

wyardley Oct 19, 2017

lukebigum commented Oct 19, 2017

alexjfisher commented Oct 24, 2017

Add boolean to control EPEL #181

Add boolean to control EPEL #181

Conversation

lukebigum commented Jul 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebigum commented Jul 12, 2017

Choose a reason for hiding this comment

lukebigum commented Jul 12, 2017

lukebigum commented Jul 21, 2017

bastelfreak commented Jul 21, 2017

lukebigum commented Jul 22, 2017

alexjfisher commented Jul 22, 2017

lukebigum commented Jul 24, 2017

tux-o-matic commented Jul 27, 2017

lukebigum commented Jul 28, 2017

tux-o-matic commented Jul 28, 2017

lukebigum commented Jul 28, 2017

tux-o-matic commented Jul 31, 2017 • edited Loading

lukebigum commented Jul 31, 2017

tux-o-matic commented Jul 31, 2017

lukebigum commented Sep 1, 2017

tux-o-matic commented Sep 4, 2017

wyardley commented Sep 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebigum commented Sep 18, 2017

ekohl commented Oct 19, 2017

lukebigum commented Oct 19, 2017

Choose a reason for hiding this comment

lukebigum commented Oct 19, 2017

alexjfisher commented Oct 24, 2017

tux-o-matic commented Jul 31, 2017 •

edited

Loading