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

Moving away from templates to usign inifile from Puppetlabs/inifile #7

Merged
merged 1 commit into from
Apr 6, 2015
Merged

Conversation

smbambling
Copy link
Contributor

This give extra flexibility when adding multiple resources or authentication types. You can just call the define in your profile manifest.

@@ -1,7 +1,7 @@
# == Class icingaweb2::config
#
class icingaweb2::config {
assert_private()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update your stdlib and revert this line. ;)

puppetlabs/puppetlabs-stdlib@bf8e5b0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The update to move to assert_private is extremely new and not included in even the newest release 4.5.1 (https://github.com/puppetlabs/puppetlabs-stdlib/tree/4.5.1). You would be required to run off the latest release or specific commit you listed above. This might limit users from being able to utilize this module, would you be ok with having it set with private() for now and updating once a new version of stdlib is cut ?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can also use is_function_available('assert_private') to support both.
It's in stdlib as well.

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 updated to support both as requested

@arioch
Copy link
Contributor

arioch commented Mar 17, 2015

Thanks for the contribution.
If you could you add spec tests I can review and merge it.

@smbambling
Copy link
Contributor Author

I unfortunately don't have any experience with writing spec tests, though my changes didn't introduce any new parameters or logic that shouldn't ( from what I gather looking at the current tests ) break or invalidate the current spec tests.

Is there something specific that you would request be added ?

@smbambling
Copy link
Contributor Author

Is there any comments you can provide or changes you think need made?

@arioch
Copy link
Contributor

arioch commented Mar 19, 2015

Unbreaking the tests would be an excellent start to get it merged. :)

$ bundle exec rake spec
Finished in 7.72 seconds (files took 0.60074 seconds to load)
77 examples, 71 failures, 6 pending

@smbambling
Copy link
Contributor Author

Well that definitely doesn't look good :), I'll try to do some reading up on rspec this week so I can debug what is making the tests fail. I'm able to manually test this with puppet apply on a host and it works, my changes must have introduced something that is making the tests freak out

@arioch
Copy link
Contributor

arioch commented Mar 19, 2015

Judging by the looks of your code I'm quite confident it works flawlessly. 👍
I could try to add the tests myself to help you out, but I don't see it happen within a week given my current time schedule.

@smbambling
Copy link
Contributor Author

Any help would be appreciated, no rush as IcingaWeb2 is still in beta and we are not actively using this module in any production environment. I was just doing some testing and wanted to give back my changes.

@ckaenzig
Copy link

ckaenzig commented Apr 2, 2015

@smbambling You first have to add a dependency on the puppetlabs-inifile module in the .fixtures.yml file:

diff --git a/.fixtures.yml b/.fixtures.yml
index a0ee02e..5e61be1 100644
--- a/.fixtures.yml
+++ b/.fixtures.yml
@@ -2,6 +2,7 @@ fixtures:
   repositories:
     apache:  'git://github.com/puppetlabs/puppetlabs-apache.git'
     concat:  'git://github.com/puppetlabs/puppetlabs-concat.git'
+    inifile: 'git://github.com/puppetlabs/puppetlabs-inifile.git'
     stdlib:  'git://github.com/puppetlabs/puppetlabs-stdlib.git'
     vcsrepo: 'git://github.com/puppetlabs/puppetlabs-vcsrepo.git'

Then they start showing a useful error:

     Puppet::Error:
       Puppet::Parser::AST::Resource failed with error ArgumentError: Invalid resource type icingaweb2::config::database at puppet-icingaweb2/spec/fixtures/modules/icingaweb2/manifests/config.pp:112

I don't see any icingaweb2::config::database defined type in our code. Have you perhaps forgot to add some file to the commit?

@smbambling
Copy link
Contributor Author

I found some typos with the resource defines and them being called in config.pp. I also added the update to the .fixtures.yml file you stated.

Now I can see some some detailed errors.

Finished in 7.76 seconds (files took 0.47417 seconds to load)
77 examples, 24 failures, 6 pending

@arioch
Copy link
Contributor

arioch commented Apr 3, 2015

Looks good so far. 👍
I still need to take a look at the 24 failing tests though.

@arioch
Copy link
Contributor

arioch commented Apr 6, 2015

Fixing those failing tests right now. :)

@arioch
Copy link
Contributor

arioch commented Apr 6, 2015

Merged upstream!

@icingaadmin icingaadmin merged commit d91ca34 into voxpupuli:master Apr 6, 2015
@smbambling
Copy link
Contributor Author

awesome thanks @arioch

MelanieGault pushed a commit to DSI-Ville-Noumea/puppet-icingaweb2 that referenced this pull request Sep 14, 2015
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.

4 participants