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 an $ensure parameter to concat #39

Merged
merged 1 commit into from
Sep 24, 2013

Conversation

FredericLespez
Copy link
Contributor

Hi,

This patch add an $ensure parameter to concat in order to control whether resources associated managed by concat should be present or absent.
This pull request also contain a format fix of concat::fragment define's declaration.

Please consider pulling.

Regards,
Fred

@ripienaar
Copy link
Contributor

Thanks, this commit breaks some of the tests, if you fix those I can take a look at merging

@FredericLespez
Copy link
Contributor Author

Sorry I didn't saw it broke the tests. I am not very familiar with rspec or Ruby :-(

I manage to setup rpec-puppet but when I run 'rake spec' at the root of concat module, here is the input:
/usr/bin/ruby1.9.1 -S rspec spec/defines/init_spec.rb --color
/usr/lib/ruby/1.9.1/rubygems/custom_require.rb:36:in `require': iconv will be deprecated in the future, use String#encode instead.
Could not load confine test 'operatingsystem': cannot load such file -- puppet/provider/confine/operatingsystem
Could not load confine test 'operatingsystem': cannot load such file -- puppet/provider/confine/operatingsystem
..........

Finished in 0.13162 seconds
10 examples, 0 failures

I don't see any failure... but I am not sure that I am doing the right thing.
Could you help me ?
Or give me your test results ?

@FredericLespez
Copy link
Contributor Author

Sorry I just saw the Travis build log...
I will look into that.

@FredericLespez
Copy link
Contributor Author

The error in Travis build should be fixed now.

@ripienaar
Copy link
Contributor

great thanks, I'll need to make some time to do manual tests too then will merge

@FredericLespez
Copy link
Contributor Author

You are welcome. Thanks to you for this module and for your great blog posts.

@FredericLespez
Copy link
Contributor Author

Oups, I find a bug.
No need for you to take time to do manuel tests.
I'll fix it and come back to you. Sorry.

@FredericLespez
Copy link
Contributor Author

Bug should be fixed.

@FredericLespez
Copy link
Contributor Author

Updated to take into account upstream commits.

@ripienaar
Copy link
Contributor

think it's right to backup all the files into the filebucket when absent or just the final managed file? As it is everything will backup, not sure if that's the best idea.

what do you think?

@FredericLespez
Copy link
Contributor Author

Good point. I am not much a user of the filebucket :-)
You are right, backing up only the final managed file is the thing to do. Other files are just temporary ones.

@apenney
Copy link

apenney commented Aug 7, 2013

@FredericLespez Hi! Any chance you can git rebase this against master for me and squash up the commits to hide the followup work that happened?

@FredericLespez
Copy link
Contributor Author

@apenney Hi ! Sure I'll try do that tomorrow.

@apenney
Copy link

apenney commented Aug 15, 2013

Any luck? I only ask as someone else asked me why we don't have ensure yet and it reminded me we have this PR :)

@FredericLespez
Copy link
Contributor Author

Sorry. More work than I thought :-/
I'll do it during the weekend.

Ashley Penney notifications@github.com a écrit :

Any luck? I only ask as someone else asked me why we don't have ensure
yet and it reminded me we have this PR :)


Reply to this email directly or view it on GitHub:
#39 (comment)

@FredericLespez
Copy link
Contributor Author

Rebased against master and commit squashed.

I just remove the modification of the CHANGELOG file since it seems you have format.

@apenney
Copy link

apenney commented Aug 19, 2013

Looks like this commit now has a duplicated resource:

  1. concat
    Failure/Error: })
    Puppet::Error:
    Duplicate declaration: File[/var/lib/puppet/concat/_etc_foo.bar] is already declared in file /home/travis/build/puppetlabs/puppetlabs-concat/spec/fixtures/modules/concat/manifests/init.pp:137; cannot redeclare at /home/travis/build/puppetlabs/puppetlabs-concat/spec/fixtures/modules/concat/manifests/init.pp:147 on node testing-worker-linux-4-1-30291-linux-1.c45665.blueboxgrid.com

    ./spec/defines/init_spec.rb:44

Before I can merge this a "rake spec" will need to pass all the tests.

@FredericLespez
Copy link
Contributor Author

Sorry. I'm looking at it now.

Ashley Penney notifications@github.com a écrit :

Looks like this commit now has a duplicated resource:

  1. concat
    Failure/Error: })
    Puppet::Error:
    Duplicate declaration: File[/var/lib/puppet/concat/_etc_foo.bar] is
    already declared in file
    /home/travis/build/puppetlabs/puppetlabs-concat/spec/fixtures/modules/concat/manifests/init.pp:137;
    cannot redeclare at
    /home/travis/build/puppetlabs/puppetlabs-concat/spec/fixtures/modules/concat/manifests/init.pp:147
    on node testing-worker-linux-4-1-30291-linux-1.c45665.blueboxgrid.com

    ./spec/defines/init_spec.rb:44

Before I can merge this a "rake spec" will need to pass all the tests.


Reply to this email directly or view it on GitHub:
#39 (comment)

This parameter controls whether ressources associated to concat should be present or absent.
@FredericLespez
Copy link
Contributor Author

I think it's good now.

@@ -46,6 +48,7 @@
# File["concat_/path/to/file"]
#
define concat(
$ensure = 'present',
Copy link
Contributor

Choose a reason for hiding this comment

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

The new $ensure param should be validated with something like validate_re($ensure, '^present$|^absent$')

@jhoblitt
Copy link
Contributor

@apenney I'm also desiring the ability to remove concat files. Are you inclined to merge this patch or do you want rspec-puppet tests with it?

@apenney
Copy link

apenney commented Sep 24, 2013

I'd love rspec-system tests more than anything else but I'm happy to merge it without. Let me regrab it and put it through the current tests at least.

apenney pushed a commit that referenced this pull request Sep 24, 2013
Add an $ensure parameter to concat
@apenney apenney merged commit bcf1f54 into puppetlabs:master Sep 24, 2013
@apenney
Copy link

apenney commented Sep 24, 2013

I've merged it but I'd still love it if anyone is brave enough to extend the spec/system tests to include ensure=present/absent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants