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

Fixes #8849 - installs foreman_gutterball #54

Merged
merged 1 commit into from
Jan 8, 2015

Conversation

dustints
Copy link

@dustints dustints commented Jan 7, 2015

No description provided.

@komidore64
Copy link

didn't need to remove anything wrt the gutterball backend service?

@ehelms
Copy link
Member

ehelms commented Jan 7, 2015

Why not include calling this in the section of init.pp that sets up gutterball? Is there ever an instance where we want to install and configure gutterball but omit the plugin for it? Or vice versa, move the configuration of gutterball into the plugin's manifest file such that if gutterball is being setup and configured the plugin does as well and then it's all nicely wrapped up in a manifest file.

@dustints dustints force-pushed the foreman_gutterball_package branch 2 times, most recently from 74385b8 to cc70dad Compare January 7, 2015 15:23
@@ -116,9 +116,12 @@

Service['httpd'] -> Exec['foreman-rake-db:seed']

class { 'katello::plugin': }

if $katello::gutterball {
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this chunk and logic in plugin/gutterball.pp ? Just makes more sense to all live together for me.

Copy link
Author

Choose a reason for hiding this comment

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

@ehelms removed the extra logic.

@dustints dustints force-pushed the foreman_gutterball_package branch 2 times, most recently from f0b4b3b to 0369c74 Compare January 7, 2015 17:27
@ehelms
Copy link
Member

ehelms commented Jan 7, 2015

I think I am getting lost in translation. I'd suggest putting all the gutterball logic in katello::plugins::gutterball and then either:

  1. In init.pp, include only:
if $katello::gutterball {
  class { 'katello::plugins::gutterball': }
}
  1. Use the installers answer file to set the plugin to true when it needs to be included and treat gutterball integration as a true plugin similar to other Foreman plugins:
katello::plugins::gutterball: true

These options let gutterball and it's integration be more cohesive and less coupled.

@@ -0,0 +1,9 @@
# gutterball plugin
class katello::plugin::gutterball{
Class[ 'certs' ] ->
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick - indentation is off with respect to other files (2 spaces instead of 4)

@dustints
Copy link
Author

dustints commented Jan 8, 2015

@ehelms added the spec but running into some issues getting a passing spec...

foreman isn't being evaluated
so foreman::plugin_prefix used in the foreman plugin type is empty causing it to install gutterball package
but gutterball module also installs the gutterball package.
so getting duplicate declaration of package 'gutterball'

do you know any way around this issue?

 [[1;31mWarning: Scope(Foreman::Plugin[gutterball]): Could not look up qualified variable 'foreman::plugin_prefix'; class foreman has not been evaluat»
^[[0;36mDebug: importing '/home/dustin/puppet-katello/spec/fixtures/modules/foreman/manifests/service.pp' in environment production^[[0m¬
^[[0;36mDebug: Automatically imported foreman::service from foreman/service into production^[[0m¬
^[[1;31mError: Duplicate declaration: Package[gutterball] is already declared in file /home/dustin/puppet-katello/spec/fixtures/modules/gutterball/man»
F¬                                                                                                                                                     
¬
Failures:¬
¬
  1) katello::plugin::gutterball should contain Class[certs]¬
     Failure/Error: it { should contain_class('certs') }¬
     Puppet::Error:¬
       Duplicate declaration: Package[gutterball] is already declared in file /home/dustin/puppet-katello/spec/fixtures/modules/gutterball/manifests/i»
     # ./spec/classes/plugin_gutterball_spec.rb:14:in `block (2 levels) in <top (required)>'¬
¬
  2) katello::plugin::gutterball should call the plugin¬
     Failure/Error: should contain_foreman__plugin('gutterball')¬
     Puppet::Error:¬
       Duplicate declaration: Package[gutterball] is already declared in file /home/dustin/puppet-katello/spec/fixtures/modules/gutterball/manifests/i»
     # ./spec/classes/plugin_gutterball_spec.rb:18:in `block (2 levels) in <top (required)>'¬
¬

require 'spec_helper'

describe 'katello::plugin::gutterball' do
let(:foreman) { {:plugin_prefix => 'ruby193-'} }
Copy link
Member

Choose a reason for hiding this comment

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

Does this work? I think we mostly use pre_condition:

let :pre_condition do
  "class {'foreman': plugin_prefix => 'ruby-193-'}"
end

Though in this particular case you don't have to specify the plugin_prefix explicitly since it should detect it from the provided facts.

@dustints
Copy link
Author

dustints commented Jan 8, 2015

thanks @ekohl, that worked!

@ehelms
Copy link
Member

ehelms commented Jan 8, 2015

ACK

dustints pushed a commit that referenced this pull request Jan 8, 2015
Fixes #8849 - installs foreman_gutterball
@dustints dustints merged commit afa0d0f into theforeman:master Jan 8, 2015
@dustints dustints deleted the foreman_gutterball_package branch January 8, 2015 19:33
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

4 participants