Skip to content

Conversation

@figless
Copy link
Contributor

@figless figless commented Aug 30, 2024

Summary

This PR fixes 1575

Additional Context

Related Issues (if any)

Checklist

I was hitting 1575 when using a Deferred password. With this fix, puppet creates the database as before (v9.2.0)

postgresql::server::db { $db_name:
      user     => $db_username,
      password => Deferred('unwrap', [$db_password]),
}

@CLAassistant
Copy link

CLAassistant commented Aug 30, 2024

CLA assistant check
All committers have signed the CLA.

@figless figless force-pushed the enum_postgresql_password branch from 396aec2 to 6084f19 Compare August 30, 2024 09:47
Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

We already have defined this as an enum:

type Postgresql::Pg_password_encryption = Enum['md5', 'scram-sha-256']

And it's used in multiple places now, so this would risk them going out of sync. I'd at least expect a comment in both places to remind people to keep them in sync.

@figless figless force-pushed the enum_postgresql_password branch 2 times, most recently from 9eb7968 to 167412d Compare October 24, 2024 13:32
@figless figless requested a review from ekohl October 25, 2024 13:43
@figless
Copy link
Contributor Author

figless commented Oct 28, 2024

We already have defined this as an enum:

type Postgresql::Pg_password_encryption = Enum['md5', 'scram-sha-256']

And it's used in multiple places now, so this would risk them going out of sync. I'd at least expect a comment in both places to remind people to keep them in sync.

I've added a comment to both files.

@bastelfreak
Copy link
Collaborator

Is there a chance you can add a test with Deferred(), to ensure this doesn't break in the future?

@figless figless force-pushed the enum_postgresql_password branch from 5609bd0 to 63eebc3 Compare October 29, 2024 14:50
@figless
Copy link
Contributor Author

figless commented Oct 29, 2024

Is there a chance you can add a test with Deferred(), to ensure this doesn't break in the future?

Sure - i've copied spec/acceptance/db_spec.rb to spec/acceptance/db_deferred_spec.rb and configured the test to use a Deferred password.

@figless
Copy link
Contributor Author

figless commented Oct 30, 2024

The previous code in db_deferred_spec.rb failed the CI, even though it was essentially a copy from db_spec.rb....
I have adapted the code, which now passes locally.

@smortex
Copy link
Collaborator

smortex commented Jan 10, 2025

I can't trigger the CI, maybe because the branch is outdated. Let's try to rebase it…

@smortex smortex force-pushed the enum_postgresql_password branch from 6f0cf55 to 7406a21 Compare January 10, 2025 21:22
@ekohl
Copy link
Collaborator

ekohl commented Jan 11, 2025

I needed to approve ci, which I just did

@figless
Copy link
Contributor Author

figless commented Jan 20, 2025

Hello,
The CI was failing on some 'puppet7-nightly' items that my PR doesn't touch.
Can the CI be retriggered?

@figless figless force-pushed the enum_postgresql_password branch 2 times, most recently from dbb02c3 to 939e909 Compare April 1, 2025 11:51
@figless
Copy link
Contributor Author

figless commented Apr 1, 2025

Hello,
I've just rebased this branch.
Can the CI please be retriggered?

bastelfreak
bastelfreak previously approved these changes Apr 17, 2025
@bastelfreak bastelfreak reopened this Apr 17, 2025
@alexjfisher
Copy link
Collaborator

alexjfisher commented Oct 22, 2025

We already have defined this as an enum:

type Postgresql::Pg_password_encryption = Enum['md5', 'scram-sha-256']

And it's used in multiple places now, so this would risk them going out of sync. I'd at least expect a comment in both places to remind people to keep them in sync.

Is it worth explicitly spelling out that type aliases can't be used in functions if you ever want the function call to be deferred? (Since types aren't plugin-synced to the agent)

@alexjfisher alexjfisher changed the title define Enum on supported encryption types for postgresql_password Don't use type alias in postgresql_password parameter Oct 23, 2025
@alexjfisher alexjfisher force-pushed the enum_postgresql_password branch from 1fe312d to 0ecebc7 Compare October 23, 2025 09:06
@alexjfisher
Copy link
Collaborator

I've rebased/squashed and tidied up the commit message.
@ekohl Would you take another look?

Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Some small implementation notes, but overall it looks ok.

Type aliases from modules aren't available on the agent.
See https://puppet.atlassian.net/browse/PUP-7197

This means that calls to `postgresql::postgresql_password` can't be
deferred if the `hash` parameter uses the type
`Postgresql::Pg_password_encryption`. Instead we have to use the raw
`Enum`, (which unfortunately causes code duplication, but this is
unavoidable).

Using this function with `Deferred` is not just a theoretical use-case.
The module itself tries to here.
https://github.com/puppetlabs/puppetlabs-postgresql/blob/20704ffae24dcf970784697b1798c09d026fb7f8/manifests/server/role.pp#L162
@alexjfisher alexjfisher force-pushed the enum_postgresql_password branch from 0ecebc7 to e57da89 Compare October 23, 2025 11:51
@alexjfisher alexjfisher requested a review from ekohl October 23, 2025 11:51
@alexjfisher
Copy link
Collaborator

Some small implementation notes, but overall it looks ok.

Fixed those up. Good to merge?

Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

My concerns have been addressed.

@alexjfisher alexjfisher merged commit 029754c into puppetlabs:main Oct 29, 2025
5 of 57 checks passed
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.

postgresql::postgres_password throws errors when Deferred

6 participants