-
Notifications
You must be signed in to change notification settings - Fork 8
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
Fix #46 (SUP-2384) #47
Conversation
rsan::exporter is a classthat may have no external impact to Forge modules. This module is declared in 0 of 576 indexed public
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MartyEwings how have you acceptance tested this? I have a feeling that this could behave differently in the following scenarios:
- Classified with
rsan::exporter
using a node group - Classified with
rsan::exporter
using static node declaration in site.pp
Sometimes it can be possible for custom classes to be evaluated before the puppet_enterprise
code which can mean that doing things like this will fail
It's been tested on a couple of installs on different versions, all node group based, but thats how rsan is to be installed. Problem was without this, it picks up the defaults from pe_postgres, sometimes which is wrong. I could always define my own params but that seems redundant |
IMO having a module make assumptions about how it's going to be classified is poor practice, people should be able to use node groups, existing roles/profiles, or any other method to classify the class and it should be able to work fine. Maybe the best compromise would be to expose these values as parameters on the class but have them default to what they are set to currently. That way if someone is doing something strange or custom, they can still get it working without a code change |
Sounds like a plan, ill push the changes |
I decided to statically define some default params, the dynamic calls to PE was gonna cause more problems that it hoped to solve |
Co-authored-by: Dylan <dylan.ratcliffe@puppet.com>
#46
Prior to this commit on some deployments of the exporter class on a postgres node, resource ordering meant that default postgres users and groups where selected instead of the puppet_enterprise defaults