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

Add options to disable repo resources when desired #54

Closed
wants to merge 1 commit into from

Conversation

rcalixte
Copy link
Contributor

@rcalixte rcalixte commented Jun 2, 2016

No description provided.

@rnelson0
Copy link
Sponsor Member

rnelson0 commented Jun 2, 2016

Is there a specific use case for this request? It's pretty verbose to wrap every resource in the module with if ! defined(). If you're using another module to manage a given yumrepo reesource, it seems like the user should probably not both modules at the same time, rather than forcing all modules to be so defensive about their coding practices.

@rnelson0
Copy link
Sponsor Member

rnelson0 commented Jun 2, 2016

You may also want to look at the epel_*_enabled params, that already exist to let you control which of the repos are enabled via this module.

@rcalixte
Copy link
Contributor Author

rcalixte commented Jun 2, 2016

The issue is when other modules have a dependency on something like (https://github.com/example42/puppet-yum) and then define another epel repo (because everyone has decided that having a different solution for implementing epel repos is necessary). I've also opened a PR with that module to try to stem off collisions at the source rather than having to chase down and modify a few dozen other modules we're using in our environment.

Essentially, this is to try to inhibit bad code from being bad during compilation. We have a single Puppetfile for our environment so all modules need to play nice together and epel is a major pain.

@stahnma
Copy link
Collaborator

stahnma commented Jun 2, 2016

It seems like the solution would be to have whatever is requiring a
specific epel module to stop that. This module can't control what uses it
as a dependency or not. The !defined methods will only work if the
resources are named identically, which certainly isn't guaranteed. On top
of that, that check happens at catalog compilation time, so without strict
flow control, one version of a catalog to another could get different
results. (If two competing resources are coded with if ! defined, which one
loads first?)

I think there's a better way. Honestly, it's a shame there isn't an
abstraction at a higher level to say something like "my stuff needs epel
and I don't care how" that many modules could satisfy.

On Thu, Jun 2, 2016 at 11:54 AM, Riccardo Calixte notifications@github.com
wrote:

The issue is when other modules have a dependency on something like (
https://github.com/example42/puppet-yum) and then define another epel
repo (because everyone has decided that having a different solution for
implementing epel repos is necessary). I've also opened a PR with that
module to try to stem off collisions at the source rather than having to
chase down and modify a few dozen other modules we're using in our
environment.

Essentially, this is to try to inhibit bad code from being bad during
compilation. We have a single Puppetfile for our environment so all modules
need to play nice together and epel is a major pain.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#54 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAAbMfKi4w6sP5DFjMS9Q1ZwoD8887Cnks5qHybVgaJpZM4Isxkt
.

@rcalixte
Copy link
Contributor Author

rcalixte commented Jun 2, 2016

The issue becomes when multiple modules have different dependencies for including/defining a yumrepo resource named epel. The catalog issue isn't much of one as they're all defining the same underlying repo (but it should compile consistently and not weave between the configuration).

To address it at a higher level would require the community to decide on one epel puppet module rather than the non-standardization.

Are you okay with the notion of including the defined checks for at least the epel repo resource?

@rnelson0
Copy link
Sponsor Member

rnelson0 commented Jun 2, 2016

We're not comfortable with that, because if you have two modules that contain a resource Yum['epel'] that are both protected with a defined() check, a user is guaranteed to receive ONE of the yum resources, but there is no guarantee WHICH yum resource is received.

@rcalixte
Copy link
Contributor Author

rcalixte commented Jun 2, 2016

Would it be better to implement this solution with a class variable for each yumrepo resource instead and wrapped each resource with that conditional?

Having no way to turn these off really sets a lot of expectations for every other module.

@rnelson0
Copy link
Sponsor Member

rnelson0 commented Jun 3, 2016

Do you mean something like 'epel_managed and 'epel_testing_managed'? That might hold value, so long as those default to true. I'm still not convinced this module is what you want if something else is providing epel repos, though.

@rcalixte rcalixte force-pushed the epelfix branch 3 times, most recently from c9eb8c9 to b842d2f Compare June 3, 2016 13:50
@rcalixte
Copy link
Contributor Author

rcalixte commented Jun 3, 2016

Those options are precisely defined now. I've updated the PR to reflect those changes.

The issue of choice comes down to other modules that have declared this module as a dependency and then yet more modules that declare other epel modules as a dependency. In our environment, we have a single Puppetfile with our modules listed and these issues have been a source of frustration for some time. Hopefully, this can address those problems.

@rcalixte rcalixte changed the title Only create objects when not otherwise defined Add options to disable repo resources when desired Jun 3, 2016
before => Yumrepo['epel','epel-source','epel-debuginfo','epel-testing','epel-testing-source','epel-testing-debuginfo'],
epel::rpm_gpg_key{ "EPEL-${os_maj_release}":
path => "/etc/pki/rpm-gpg/RPM-GPG-KEY-EPEL-${os_maj_release}",
before => Yumrepo['epel','epel-source','epel-debuginfo','epel-testing','epel-testing-source','epel-testing-debuginfo'],
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

@stahnma Should this stanza work if, say, 'epel-debuginfo' is not in the catalog? That's my only concern, and we'll have to add a test for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could add this condition further up in the init.pp outside of the resource definition. I'm thinking something along the lines of
Epel::Rpm_gpg_key -> Yumrepo

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Using spaceship collectors? I think that would be better, and more future proof.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the code yet again. I'm going to test it as well on a few nodes.

@rnelson0
Copy link
Sponsor Member

Merged in #68. Sorry this took so long to accept, and thank you for your contribution!

@rnelson0 rnelson0 closed this Oct 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants