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

Fix #46 (SUP-2384) #47

Merged
merged 3 commits into from Apr 27, 2021
Merged

Fix #46 (SUP-2384) #47

merged 3 commits into from Apr 27, 2021

Conversation

MartyEwings
Copy link
Collaborator

#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

@MartyEwings MartyEwings added the bug Something isn't working label Apr 26, 2021
@puppet-community-rangefinder
Copy link

rsan::exporter is a class

that may have no external impact to Forge modules.

This module is declared in 0 of 576 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@MartyEwings MartyEwings changed the title Fix #46 Fix #46 (SUP-2384) Apr 26, 2021
Copy link
Contributor

@dylanratcliffe dylanratcliffe left a 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

@MartyEwings
Copy link
Collaborator Author

MartyEwings commented Apr 26, 2021

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

@dylanratcliffe
Copy link
Contributor

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

@MartyEwings
Copy link
Collaborator Author

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

@MartyEwings
Copy link
Collaborator Author

I decided to statically define some default params, the dynamic calls to PE was gonna cause more problems that it hoped to solve

manifests/exporter.pp Outdated Show resolved Hide resolved
Co-authored-by: Dylan <dylan.ratcliffe@puppet.com>
@MartyEwings MartyEwings merged commit 050cbc4 into main Apr 27, 2021
@MartyEwings MartyEwings deleted the (SUP-2384) branch April 29, 2021 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(SUP-2384) Resource ordering issue with Rsan::Exporter/Pe_postgresql_psql
2 participants