-
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
W.I.P. PR2: New functions created to query for each item - calling of functions in importer.pp #10
Conversation
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.
Looks great. Only thing I would change is making the variables parameters so that it's easy to customise
manifests/importer.pp
Outdated
|
||
#in the below class | ||
|
||
$puppet_severs = rsan::get_puppet_servers() |
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.
Can we expose these as parameters instead of just declaring as variables? That way someone should be able to call the class like so and override the defaults if they need to:
class { 'rsan::importer':
puppet_servers => ['server1.company.com', 'server2.company.com'],
}
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.
So in this instance, I'd be looking to do something like:
class { 'rsan::importer':
puppet_servers => $puppet_servers,
puppetdb_hosts => $puppetdb_hosts,
postgres_hosts => $postgres_hosts,
}
Is that right?
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.
Well that's what the user would do when the use RSAN. In order for you to allow them to do that you'll need to allow the rsan::importer
class to accept parameters that the user passes i.e.
class rsan::importer (
String $some_param = "hello",
) {
functions/get_postgres_hosts.pp
Outdated
@@ -0,0 +1,18 @@ | |||
function rsan::get_postgres_hosts() { | |||
if $settings::storeconfigs { |
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.
this makes this whole thing conditional on $settings::storeconfigs, which is a snippet from another place in code this was copied from. This does not need to be conditional
functions/get_postgres_hosts.pp
Outdated
expired is null | ||
} | ||
}').map |$data| { $data['certname'] } | ||
} else { |
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.
as there is no longer a conditional the exception of not having a postgres host returned needs handled differently
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.
Hey Marty, would this mean I need to implement some sort of error catch? So if puppetdb_query returns nothing/an error - make an exception to set
$postgres_hosts = []
functions/get_postgres_hosts.pp
Outdated
} | ||
}').map |$data| { $data['certname'] } | ||
} else { | ||
$postgres_hosts = [] |
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.
$postgres_hosts would need to be returned in a format
puppet_metrics_dashboard:: postgres_host_list expects
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.
So in this case, puppet_metrics_dashboard::postgres_host would need to be an array of elements, with each element being a postgres host. I've been looking at https://puppet.com/docs/puppet/7.0/lang_data_type.html
Initially, part of me thinks that I need to some thing like:
function rsan::get_postgres_hosts() {
$postgres_hosts => Array #this is the bit im thinking of adding in
$postgres_hosts =
puppetdb_query('resources[certname] {
type = "Class" and
title = "Pe_postgresql::Server::Install" and
nodes {
deactivated is null and
expired is null
}
}').map |$data| { $data['certname'] }
}
My thinking is that by setting the data type beforehand - puppetdb_query will fill in the elements to that array as appropriate?
But I've also come across https://puppet.com/docs/puppet/7.0/lang_data_type.html#lang_data_type_usage
Where there is the 'Type' function. In this instance, I would do something like get the data type from the $postgres_hosts after the puppetdb_query is finished, then if the data type of $postgres_hosts =/ array, I set it as so?
@@ -0,0 +1,11 @@ | |||
function rsan::get_puppet_servers() { | |||
$puppet_servers = |
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.
there is a syntax issue here, best PDK validate
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.
Syntax issue on this should be solved in my next commit from my local repo :)
@@ -0,0 +1,11 @@ | |||
function rsan::get_puppet_servers() { | |||
$puppet_servers = |
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.
$puppet_servers needs to be in a format
puppet_metrics_dashboard::master_list expects
@@ -0,0 +1,17 @@ | |||
function rsan::get_puppetdb_hosts() { | |||
if $settings::storeconfigs { |
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.
same comments on the condition here
} | ||
}').map |$data| { $data['certname'] } | ||
} else { | ||
$puppetdb_hosts = [] |
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.
puppetdb_hosts has to be returned in the correct format for
puppet_metrics_dashboard::puppetdb_list
This functionality already exists in the Also, the postgres query does not work because it doesn't capitalize each element of the class name. |
@m0dular Thanks for the input, that does look like a good solution. The PRS here are for review and collaboration and are not meant to be a final code. Would you like to added to the codeowners for review or added to the project? |
Hey guys,
In this pull request:
Added function to query for puppet servers on the estate
Added function to query for puppetdb_hosts on the estate
Added function to query for postgres_hosts on the estate (this one I'm not too confident on, essentially copied the code for querying the puppetdb_hosts, but changed to look for nodes with the class pe_postgresql::server::install attached)
Changed the code in importer.pp to call each of these functions and save them to their respective variable names, then calling these variables in the puppet_metrics_dashboard class