fix postgres module to use config.get

[nicholas-rees]

What does this PR do?

Modifies the postgres execution module to respect the configuration set by changing all calls from config.option to config.get and setting the string delimiters to colons.

As noted in the related issue I opened:

# salt-call config.option

Passed invalid arguments: option() missing 1 required positional argument: 'value'.

Usage:

Returns the setting for the specified config value. The priority for
matches is the same as in :py:func:`config.get <salt.modules.config.get>`,
only this function does not recurse into nested data structures. Another
difference between this function and :py:func:`config.get
<salt.modules.config.get>` is that it comes with a set of "sane defaults".
To view these, you can run the following command:

It seems like config.option is just the wrong function to use and config.get is the correct one.

Before this fix:

# salt-call postgres.psql_query "SELECT current_user, session_user;"
local:
    |_
      ----------
      current_user:
          postgres
      session_user:
          postgres

After:

# salt-call postgres.psql_query "SELECT current_user, session_user;"
local:
    |_
      ----------
      current_user:
          salt_admin
      session_user:
          salt_admin

What issues does this PR fix or reference?

Fixes #69971

Previous Behavior

postgres module was not respecting the configuration set in either the pillars or minion configuration file.

New Behavior

postgres module now respects configuration values.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

• Docs
• Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
• Tests written/updated

Commits signed with GPG?

Yes

[nicholas-rees]

@twangboy - With respect to the feedback on the previous MR:

Please help me understand. Using delimter='.' is a coding standard and deviating can break documentation. Makes sense (not really following how it can break existing keys, because currently the configuration isn't being respected anyway, but I think this is a digression).

However, as far as I can tell, what the postgres module really does is look for the psql binary on the system and execute it. As a result, I am unable to identify any configuration options that should have non-None default values. In fact:

    if user:
        cmd += ["--username", user]
    if host:
        cmd += ["--host", host]
    if port:
        cmd += ["--port", str(port)]
    if not maintenance_db:
        maintenance_db = "postgres"

postgres.bins_dir also has a similar check, and postgres.timeout already has a default set. I do not believe that postgres.pass ought to have a default as well.

The above code block seems to support that every config option ought to be able to accept non-None values. Please inform me of any errors in my understanding, so I can make the correction. In fact, it looks to me like it is expected and designed that configuration values ought to return None if not set (probably why it has not been noticed that config.option does not respect the nested keys for quite some time).

The only way I can think of to make sense, is to make the defaults of config values to return False, but that seems a little heavy handed in my opinion. Either, way, if that solution is acceptable, then I can make that change as well.

[twangboy]

I think you're right. The connection logic in postgres.py checks for Falsey values to conditionally build CLI flags so passing default=... isn't necessary there. Appreciate you walking through that.

The key reason we need delimiter="." is to prevent breaking existing minion and pillar configurations across upgrades.

Many deployments currently set flat keys as documented in the module header:

postgres.host: 'localhost'
postgres.user: 'salt_admin'
postgres.bins_dir: '/usr/pgsql-14/bin'

Because config.get defaults to : as its nested delimiter, calling config.get("postgres:user") strictly expects a nested dict (postgres: { user: ... }) and skips over flat postgres.user keys entirely.

Using delimiter=".":

__salt__["config.get"]("postgres.user", delimiter=".")

allows Salt to evaluate both flat postgres.user keys and nested postgres: { user: ... } structures seamlessly.

Looks like there's a small typo in the parameter name. delimiter is misspelled as delimeter (with an i instead of an e) across the config.get calls (e.g. lines 143, 160, 166, 181, 347, etc.):

# Current (typo in parameter name):
pg_bin_dir = __salt__["config.get"]("postgres.bins_dir", delimeter='.')

# Needs to be:
pg_bin_dir = __salt__["config.get"]("postgres.bins_dir", delimiter='.')

Additionally, this will need a changelog and some tests written.

Thank you for rebasing on 3006.x.

[nicholas-rees]

@twangboy Thank you for your explanation. My spelling isn't the best, I did notice the mispelling, and I thought that I pushed it forward, but I'm working with several different machines, because I can't get the pre-commits to work on my mac (I think I would like to try to open with an issue and pr for that as well if it is welcome, but one thing at a time).

I see what you're saying about the yaml now, I thought that was another way of nesting keys in yaml, but now that you have said something I'm able to see that I was wrong. Thanks for helping me out.

I have pushed the fix to the typo with delimiter.

I'm not sure about the changelog, but I could work on testing if I could get a tad bit of direction with the changelog and tests.

The contribution guide says

s a contributor, all that means is that you need to add a file to the salt/changelog directory, using the <issue #>..md format.

However, under salt there is no changelog, except for at the root of the project, so I've put it there (I think that's right, just want to bring up my confusion).

I think you might need to give me a bit until I can understand what your testing process is. I'm going to read: https://docs.saltproject.io/en/master/topics/tutorials/writing_tests.html. Is that a the best documentation on your expectations about tests?

Would you like me to change this PR to WIP until the tests are written? (Honestly, I was kind of hoping that some tests for this was already written but I'm happy to help fill in that gap if there is an opportunity for that).