Feature inv/lookup

[gebz97]

SUMMARY

1. Inventory plugin using PostgreSQL as the dynamic inventory source
2. Lookup plugin for pulling data from a PostgreSQL table

ISSUE TYPE

• New Plugin Pull Request

COMPONENT NAME

1. postgresql_inventory
2. postgresql_lookup

ADDITIONAL INFORMATION

The purpose of the plugins are pretty straight forward, I am unsure however if it is a must for me to include integration tests for the lookup plugin (I did include 26 unit tests for the inventory plugin).
(this is my first contribution to this project, so please be patient with me :) )

[Andersson007]

@gebz97 hello, thanks for the PR

I see issues that need to be solved, but let's not review the code and first discuss whether we need the plugins as it's big chunks of code that need to be maintained. I wouldn't expand the collection unless proposed features would be really helpful for a broad audience.

As I'm not a user, @andreasscherbaum @hunleyd @toydarian and others, please share your thoughts.

[toydarian]

Full disclosure: I haven't looked at the code yet.

I see issues that need to be solved, but let's not review the code and first discuss whether we need the plugins as it's big chunks of code that need to be maintained. I wouldn't expand the collection unless proposed features would be really helpful for a broad audience.

I can definitely see how this would be useful. But I don't know about the broader audience. Maybe @gebz97 can shine some light on that?
Regarding the maintenance, I would definitely want to see integration tests, as that is an amount of code that for sure hides some bug somewhere. Integration tests are not necessarily there to show that the code doesn't contain issues at the moment, but more to ensure we don't break anything when we make changes later.
I would also suspect that there are quite a few edge-cases that will come up if other people than the author use those plugins.

[andreasscherbaum]

First of all, I suggest you submit both modules as separate PRs. That makes reviewing and testing a lot easier.

Like others, I'd like to see a few real-world use cases for both modules. Once this is merged, it must be maintained and that requires constant updates. If you are the only user then this depends on you.

Regards tests:
Why are there two files tests/unit/plugins/inventory/inventory.pg_inv.yaml and a tests/unit/plugins/inventory/inventory.pg_inv.yml?

The list of hosts and groups in the tests is ... sparse. This needs a lot more variations both in hosts and groups. And tests.

A password for database access is not always necessary, pg_hba.conf can allow access based on other rules, like IP address.

For the inventory plugin:
Managing groups as array or text is unfortunately less than effective. Updating groups requires complex text manipulations.
This rather be a separate table for groups, and then additionally 1:n tables for the relations between hosts and groups.
The same can be said about host_vars. Updating any host var will end up in a mundane task of writing complicated SQL.

I would NOT depend on a provided SQL query at all, but pre-define table and column names. Makes it much easier.

For the inventory plugin:
Neither 'query' nor 'terms' are listed in the documentation of the plugin.
At first glance I don't even know how to use this plugin.
Like for the inventory plugin: why not go with static table and column names?

While writing this review I was thinking hard how I could possibly use these two plugins.
Having a database holding the inventory asks for trouble, as somehow this database must be installed and maintained.
That's a job for Ansible. Which results in a chicken-egg problem.

[gspanos]

1. As a general suggestion, I think a PR should contain only 1 feature and not multiple. If I'm not mistaken, these are 2 unreleated features. This makes it hard to discuss, review, test, release or track possible regressions.
2. I think the inventory plugin with Postgres as backend, makes some sense (especially taking into consideration that an equivalent for AWS RDS already exists here: https://docs.ansible.com/ansible/latest/collections/amazon/aws/aws_rds_inventory.html )
3. I understand the concept of a Postgres lookup plugin, I just can't think of a real world example that would make sense. @gebz97 can you provide some real-world cases for this maybe?

[gspanos]

@andreasscherbaum

Having a database holding the inventory asks for trouble, as somehow this database must be installed and maintained.
That's a job for Ansible. Which results in a chicken-egg problem.

Not sure exactly why is this a problem. Can't 2 playbooks have separate inventories? So you could have a static inventory for setting up the PostgreSQL and then, for the rest of the playbooks, rely on the PostgreSQL-based one?

[andreasscherbaum]

Not sure exactly why is this a problem. Can't 2 playbooks have separate inventories? So you could have a static inventory for setting up the PostgreSQL and then, for the rest of the playbooks, rely on the PostgreSQL-based one?

Yes, you can. This falls into the wider question about use cases which was not only raised by me.

[gebz97]

Hello everyone,

Thanks for the feedback, I really appreciate it.

I will close this PR for now, but will definitely discuss all the points raised in the comments first on the matrix channel, and will proceed with creating separate PRs for each feature along with the justification/use cases.

If someone is interested in sharing insights on the design of the plugins I would appreciate it, as this is a first time for me.

[Andersson007]

Great discussion, folks, thanks! Here's a link to the forum post https://forum.ansible.com/t/plugin-idea-postgresql-inventory/44621/3

My 2c (writing this being heavily underslept. sorry if there's confusion):

• As others pointed out, there must be separate PRs for multiple not tightly-related entities
• There must also be comprehensive integration tests coverage in addition to the unit tests
• About use cases, as I mentioned, the ideas about PostgreSQL lookup/inventory popped up in my head many years ago:
  • I failed to come up with potentially-widely-popular use cases for them:
    • lookup: can't we use postgresql_query to fetch anything from a local or remote DB in a very flexible way by passing any query we want? Then we can transform the output to whatever format we need using already available filters, etc.
    • inventory: I guess predefined tables or column names is something very opinionated and fragile:
      • again, can't postgresql_query fetch that info and pass it to inventory? (I'm not a strong user though)
      • users will probably need to create and update such tables manually. If this is the case, what's the difference with maintaining an inventory file manually?
      • if some kind of infra management app is supposed to update those tables, are there many users who want to (re-)write such apps to support those custom tables / columns?
  • There never were requests for them from the community
  • As one of maintainers of this collection, I'm thinking about maintenance of things we merge (but I'm personally OK with it, if the community want it much)