Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add tags patroni fix #40

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hache
Copy link

@hache hache commented Oct 2, 2024

Based in our previous collaborator work.
-Fixed space.
-Removed expected tags parameters.
Im using patroni managed by puppet so I would like to move forward this change.
If there is something alse I should do, please tell me.

@hache
Copy link
Author

hache commented Oct 3, 2024

@ghoneycutt could you or any authorised collaborator have a look? I would like to move things forward. If there is something else I should do, please tell me.
Thank you.

@@ -328,6 +328,15 @@
end
end

context 'tags => true' do
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be tests for all potential changes here.

What if

tags = true and tags_parameters is empty?
tags = true and tags_parameters has values?
tags = false and tags_parameters is empty?
tags = false and tags_parameters has values?

@@ -239,6 +239,10 @@
# @param service_enable
# Patroni service enable property
# @param custom_pip_provider
# @param tags
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add an acceptance test that shows when this is set to true and has tags_parameters that the correct information is in the yaml config.

Copy link
Contributor

@ghoneycutt ghoneycutt left a comment

Choose a reason for hiding this comment

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

See notes

@ghoneycutt
Copy link
Contributor

Thank you very much for these contributions!

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants