Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

Switching to in module hiera data & updating docs #65

Merged
merged 1 commit into from
Jul 10, 2019

Conversation

genebean
Copy link
Contributor

@genebean genebean commented Jul 9, 2019

This PR does the following:

  • Switched from Hiera backend to standard function
  • Created a function for determining the needed PuppetDB metrics based
    on PE version
  • Removed the variable 'storage_metrics_db_queries' since it was not
    referenced anywhere.
  • Updated the README and documentation in some manifests

This all stemmed from trying to solve the problem of having to define
the same values for parameter defaults in multiple places. The part of
this PR that addresses that exact problem is the new
puppet_metrics_dashboard::puppetdb_metrics() function. The rest of this
was just a by-product of getting to that solution.

@genebean genebean requested a review from suckatrash July 9, 2019 15:19
Copy link
Contributor

@suckatrash suckatrash left a comment

Choose a reason for hiding this comment

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

This looks great! Just one question below

functions/puppetdb_metrics_params.pp Outdated Show resolved Hide resolved
Copy link
Contributor

@npwalker npwalker left a comment

Choose a reason for hiding this comment

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

Does module_data work for users still on hiera 3?

I think the function is overkill and makes figuring out defaults significantly more complicated than just leaving it as it was. I believe the best practice on module data allows for still using params.pp when it is inconvenient to use module data.

@genebean
Copy link
Contributor Author

genebean commented Jul 9, 2019

The problem being solved here @npwalker is centered around the user not having to define the puppetdb_metrics in their own hiera as @suckatrash was unable to get a defined type to use what was in params.pp.

@genebean
Copy link
Contributor Author

genebean commented Jul 9, 2019

@suckatrash
Copy link
Contributor

@npwalker Docs tell me "You can even start using some Hiera 5 features—like module data—without upgrading anything." ...I forget how that works exactly, but it's notable that the puppetlabs-ntp module doesn't use params.pp at all.

As @genebean mentioned, this solves an important problem: we need parameter defaults to be available in one of the defined types puppet_metrics_dashboard::profile::puppetdb (when it's applied outside of any other class in this module) and there doesn't seem to be a great way to do that without a solution like this.

@npwalker
Copy link
Contributor

npwalker commented Jul 9, 2019

Ah the reason for the change isn't clear. If it were me I would have just made two functions, one to return the timeout and one to return the puppetdb_metrics hash instead of going into module_data and involving hiera.

For puppetdb_metrics I think that's still the correct solution. There's seemingly no reason to involve hiera to get that hash when you can just call the function directly.

@suckatrash
Copy link
Contributor

The reason I like having the default params in hiera is that it's far easier to access the values without including the params class. With the defined types being introduced in #62, we might want to access other default values in the future, and module_data just seems the way to go to me.

This commit does the following:

- Switched from Hiera backend to standard function
- Created a function for determining the needed PuppetDB metrics based
  on PE version
- Removed the variable 'storage_metrics_db_queries' since it was not
  referenced anywhere.
- Updated the README and documentation in some manifests

This all stemmed from trying to solve the problem of having to define
the same values for parameter defaults in multiple places. The part of
this commit that addresses that exact problem is the new
puppet_metrics_dashboard::puppetdb_metrics() function. The rest of this
was just a by-product of getting to that solution.
@genebean genebean changed the title Switching to in module hiera data Switching to in module hiera data & updating docs Jul 10, 2019
@genebean
Copy link
Contributor Author

I believe I am finished reworking this PR. I have also updated the commit message and the top comment.

@suckatrash suckatrash merged commit 5f511ea into puppetlabs:master Jul 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants