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

Conflict between r10k subclasses and pe_r10k due to require r10k in subclasses #323

Closed
rnelson0 opened this issue Jan 6, 2017 · 2 comments
Assignees

Comments

@rnelson0
Copy link
Member

rnelson0 commented Jan 6, 2017

In init.pp:

  # Check if user is declaring both classes
  # Other classes like r10k::webhook is supported but
  # using both classes makes no sense unless given pe_r10k
  # overrides this modules default config
  if defined(Class['pe_r10k']) {
    fail('This module does not support being declared with pe_r10k')
  }

The following config at one time worked on a master:

include ::pe_r10k
include ::r10k::mcollective
class {'r10k::webhook::config': }
class {'r10k::webhook': }

Both webhook.pp and mcollective.pp received the statement require ::r10k during the initial modulesync when migrated from acidprime, here and here respectively.

Investigate if the require is ahem required or if it was added in error. This is related to #282 and should be included in the same release as #268.

@esalberg
Copy link

esalberg commented Jan 6, 2017

Thank you! Let me know if there's anything I can do to help.

One thought I had was that was maybe a conditional:

if !defined(Class['pe_r10k']) {
require ::r10k
}

But I didn't know if you'd want to include even more refrences to pe_r10k in your code. Technically, I'd think you'd want to have either pe_r10k or r10k defined when using r10k::mcollective and/or r10k::webhook?

@rnelson0
Copy link
Member Author

rnelson0 commented Jan 6, 2017

The original commits contributing to this added $root_user and $root_group back at d1fe76d. I think by namespacing references properly the requires can be removed.

@rnelson0 rnelson0 self-assigned this Jan 6, 2017
@rnelson0 rnelson0 added the needs-work not ready to merge just yet label Jan 6, 2017
@rnelson0 rnelson0 removed the needs-work not ready to merge just yet label Jan 6, 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

No branches or pull requests

2 participants