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

Ordering #45

Closed
wants to merge 1 commit into from
Closed

Ordering #45

wants to merge 1 commit into from

Conversation

rnelson0
Copy link

This addresses the ordering in #43 and prevents errors.

@rnelson0
Copy link
Author

rnelson0 commented Feb 4, 2015

@thias Have you had a chance to review this? Thanks!

@rnelson0
Copy link
Author

@thias Bump for review

@thias
Copy link
Owner

thias commented Nov 16, 2015

Sorry for the looooong delay. This probably works fine when using hiera for class parameter lookup, but will break badly with the main class declared directly from puppet manifests with explicit parameter values. It also only works when you do not have a packagenamesuffix, and breaks when you do.

Wouldn't it be easier and overall better to just add require => Class['::bind::package'] to the resources which need the package to be installed?

@rnelson0
Copy link
Author

As is, no, because the class is not contained. The class can be required but the package could come later. The alternative would be to create proper containment so that the entire class including the package resource is always processed prior to the resources that require the class. I am not sure how to use contain effectively, though.

@thias
Copy link
Owner

thias commented Nov 16, 2015

Ah yes, and anchors, etc. yuck.

So maybe set an alias of 'bind' on the package and just use require => Package['bind']? I'm already doing that in other modules for the same reasons.

@rnelson0
Copy link
Author

That would probably work, I can see about trying that in a new PR tonight.

Rob Nelson
rnelson0@gmail.com

@rnelson0
Copy link
Author

@thias So "tonight" came and went and then a few more, but I'm back on this :) Do you have an example of where you've done aliasing of packages before? I'm pretty sure it would be like so, but would love to see it in action elsewhere:

package { 'bind':
  ensure => present,
  name => $packagenameprefix,    # or the local version of $::bind::params::packagenameprefix
}

@rnelson0
Copy link
Author

rnelson0 commented Feb 6, 2016

@thias Bump. Just looking for an example so I can write an accurate PR.

@thias
Copy link
Owner

thias commented Feb 8, 2016

An example could be my SELinux module, with the audit2allow package. It's pretty much what you wrote.

@rnelson0 rnelson0 mentioned this pull request Feb 9, 2016
  Defined type bind::server::conf includes its dependency bind.
  Add $packagenameprefix to bind::server::conf from bind::params.
  Add rspec tests for three known OS families and one default value.
  Add a default .gitignore.
@rnelson0
Copy link
Author

@thias Sorry for the loooong delay on this. I have the packagename from params, and the commits have been squashed. This is ready for review now.

@rnelson0
Copy link
Author

@thias Also #46 is outstanding, not sure if you want to merge that first so we can accurately see what this might break in different versions, rather than just the puppet version I tested with locally.

@rnelson0
Copy link
Author

@thias bump

@rnelson0
Copy link
Author

@thias Have you had a chance to review this?

@thias
Copy link
Owner

thias commented Feb 2, 2017

Thanks a lot for your contributions, but no, I still haven't had time to properly review this.

The main reason is because all of my modules are focused on my own needs/environments, and "work for me"... from there I have only a limited amount of time to invest on them. The other reason is because adding features adds maintenance overhead... and the puppet rspec part is a good example : What's inside this module wasn't created by me, and I have a hard time maintaining it because I don't use proper tests as much as I should and because each time I look into them, things have changed all over the place :-/

Sorry... I have just released 0.5.3 with a single minor change, and still hope to be able to review the main pending PRs and issues, but I have no idea when that might be.

@thias
Copy link
Owner

thias commented Apr 23, 2018

I'll go with the simpler fix from #71 for now.

@thias thias closed this Apr 23, 2018
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

2 participants