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

Role Valid Until Date #1624

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

mooreandrew
Copy link
Contributor

@mooreandrew mooreandrew commented Dec 18, 2024

Summary

I have a requirement to set valid until dates for roles that I need to manage via this puppet postgresql module. This change adds support for setting a valid_until attribute for a role.

Related Issues (if any)

None

Checklist

  • 🟢 Spec tests.
  • 🟢 Acceptance tests.
  • Manually verified. (For example puppet apply)

@CLAassistant
Copy link

CLAassistant commented Dec 18, 2024

CLA assistant check
All committers have signed the CLA.

@smortex
Copy link
Collaborator

smortex commented Dec 24, 2024

See CI failures about missing documentation. Also, adding tests to spec/defines/server/role_spec.rb to ensure the feature don't break in the future would be nice.

@mooreandrew
Copy link
Contributor Author

@smortex I've updated the code, adding documentation for the variable and tests to spec/defines/server_instance_spec.rb in the same way as the other alter commands.

Let me know if that is good or if you need me to make any further changes.

This allows for the valid until attribute to be set on roles.
@smortex smortex force-pushed the feature/support-role-valid-until branch from c9adde5 to c16d850 Compare January 7, 2025 23:50
@mooreandrew
Copy link
Contributor Author

@smortex The pipeline all appears to look good. How can I got this change merged?

@alexjfisher
Copy link
Collaborator

Looks okay to me too. Only query is should this have a mechanism for removing the expiry date? eg. perhaps if the parameter is set to a special value such as the string 'absent' or maybe the boolean false (in which case you'd probably need unless $valid_until == undef { instead of if $valid_until { for your conditional), then rolvaliduntil should be set to NULL (unless it's already NULL)

@smortex
Copy link
Collaborator

smortex commented Jan 10, 2025

Looks okay to me too. Only query is should this have a mechanism for removing the expiry date? eg. perhaps if the parameter is set to a special value such as the string 'absent' or maybe the boolean false (in which case you'd probably need unless $valid_until == undef { instead of if $valid_until { for your conditional), then rolvaliduntil should be set to NULL (unless it's already NULL)

That would make sense, @mooreandrew feel free to tackle this if you want to. In the meantime, this PR is fine so I'll merge it as it is now.

Thanks!

@smortex smortex merged commit d5911ec into puppetlabs:main Jan 10, 2025
58 checks passed
@alexjfisher
Copy link
Collaborator

Looks okay to me too. Only query is should this have a mechanism for removing the expiry date? eg. perhaps if the parameter is set to a special value such as the string 'absent' or maybe the boolean false (in which case you'd probably need unless $valid_until == undef { instead of if $valid_until { for your conditional), then rolvaliduntil should be set to NULL (unless it's already NULL)

That would make sense, @mooreandrew feel free to tackle this if you want to. In the meantime, this PR is fine so I'll merge it as it is now.

Thanks!

Yeah, I'm fine with that. This MR is safe as-is, and the additional functionality can be added later in a non disruptive way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants