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

Fetch installed PSQL version #275

Merged
merged 6 commits into from
Jun 28, 2022
Merged

Fetch installed PSQL version #275

merged 6 commits into from
Jun 28, 2022

Conversation

ody
Copy link
Member

@ody ody commented Jun 22, 2022

Provides task that reuses code from facter to return the currently major version of the installed PSQL server. Data is used to construct paths to specific files required for different plans.

@ody
Copy link
Member Author

ody commented Jun 22, 2022

Should we just use facter? This is lighter weight and "should" always work as long as Puppet agent has been ran once, which it always is.

require '/opt/puppetlabs/puppet/cache/lib/pe_install/pe_postgresql_info.rb'

# GetPEAdmConfig task class
class GetPSQLInfo
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't use the ruby task helper, because it doesn't need param processing?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't think that through, just copied the structure of an existing similar task peadm::get_peadm_config

Copy link
Member Author

Choose a reason for hiding this comment

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

Other thought. Using the ruby_task_helper would also require it as a dependency while not providing any additional value

Copy link
Contributor

Choose a reason for hiding this comment

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

@ody Understood, but in that case - isn't it overkill to create a Ruby class instead of just using 3 lines of procedural code?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are correct. The usage of a Ruby class is a simple side effect of my copy and pasting existing code.

Copy link
Contributor

Choose a reason for hiding this comment

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

The class wrapper is present in the pattern in order to enable unit testing, should unit tests be written.

We should probably just adopt ruby_task_helper instead. It's better to standardize on a standard rather than create our own inside PEAdm. If we do that, we also gain the ability to write unit tests for tasks.

ody added 2 commits June 23, 2022 21:06
Reuses library from facter that can report the major version of PSQL
installed
Ues get_psql_info to dynamically set the path to files required for
setting up authentication
@ody ody force-pushed the get_psql_version branch from 93fab1b to 8724708 Compare June 23, 2022 21:10
The possible values for the clientcert setting changed between 11 and
14, use get_psql_info to determine setting based on version
@ody ody force-pushed the get_psql_version branch from 8724708 to 51b0dc3 Compare June 23, 2022 21:12
@ody ody marked this pull request as ready for review June 23, 2022 21:15
@ody ody requested a review from a team as a code owner June 23, 2022 21:15
Copy link
Contributor

@timidri timidri left a comment

Choose a reason for hiding this comment

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

LGTM

@ody ody force-pushed the get_psql_version branch 3 times, most recently from d62e6c3 to 077d6ac Compare June 28, 2022 00:12
ody added 3 commits June 28, 2022 00:33
It was determined that original pattern did not provide any additional
value over more established ones.
Actions taken on $replica_postgresql_host were previously a noop when
the parameter was not provided because the tasks would be ran with an
undef set of targets. Doing this when introducing get_psql_version
results in an error because you can't run functions against an undef
return value if the task was a noop.

Wrap everything related to $replica_postgresql_host in an if statement
to prevent errors when the parameter is not provided.
The add_compiler plan makes changes to PSQL for communication with
PuppetDB.
@ody ody force-pushed the get_psql_version branch from 077d6ac to 076cc7f Compare June 28, 2022 00:34
@ody
Copy link
Member Author

ody commented Jun 28, 2022

@timidri @reidmv Re-written to use ruby_task_helper provided Reid's comment on why the pattern was being used in peadm. Found a couple additional bits while re-testing too.

Copy link
Contributor

@timidri timidri left a comment

Choose a reason for hiding this comment

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

LGTM 2 :-)

@reidmv
Copy link
Contributor

reidmv commented Jun 28, 2022

We'll need to add ruby_task_helper to the dependencies in metadata.json if it's being used now. I think this would be the first actual use of it (though we should be using it elsewhere).

@ody
Copy link
Member Author

ody commented Jun 28, 2022

@reidmv I did.

@reidmv
Copy link
Contributor

reidmv commented Jun 28, 2022

@ do'h! yep, you did. I totally missed that. Take my reviewer approval. 😂

@ody ody merged commit b68da38 into puppetlabs:main Jun 28, 2022
@ody ody deleted the get_psql_version branch June 28, 2022 17:56
@ody ody linked an issue Aug 29, 2022 that may be closed by this pull request
@ody ody added bug bugfix and removed bug labels Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plans do not consider differences between PSQL 11 and 14
3 participants