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

Fixed undefined var issues in the create.yml and delete.yml files of azure_manage_postgresql role #96

Merged

Conversation

prabinovRedhat
Copy link
Collaborator

Fixed bugs:

@@ -64,7 +64,7 @@
value: "{{ item.value }}"
with_items: "{{ azure_manage_postgresql_postgresql_settings }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't we still have an error here if azure_manage_postgresql_postgresql_settings is not defined?

I think a better solution would be to set this in defaults/main.yml

azure_manage_postgresql_postgresql_settings: []

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think in case of undefined var it won't even come to this line, will fail on "when". But I'll set in defaults/main.yml , thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be able to remove the default() calls now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure, sorry. I thought I already pushed it, my fault..

@prabinovRedhat prabinovRedhat force-pushed the fix_undefined_vars_postgresql branch from 77e929f to 6b9b675 Compare September 11, 2024 14:20
Copy link
Contributor

@p3ck p3ck left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -1,3 +1,4 @@
---
azure_manage_postgresql_operation: create
azure_manage_postgresql_postgresql_version: "11"
azure_manage_postgresql_postgresql_settings: []
Copy link
Contributor

Choose a reason for hiding this comment

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

I think instead of adding empty default we should handle the "not defined" case in the role itself.
It should be done by adding the following lines:

  when:
    - azure_manage_postgresql_postgresql_settings is defined
    - azure_manage_postgresql_postgresql_settings | length > 0

What do you think @p3ck @prabinovRedhat ?
This is something we should change in multiple places, so I'm ok with fixing it here now or leaving it with the default solution and opening new task for this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can add it , I'll update the PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The PR is updated, @nirarg @p3ck

@prabinovRedhat prabinovRedhat force-pushed the fix_undefined_vars_postgresql branch from f2b0d20 to 363b268 Compare September 12, 2024 11:29
@prabinovRedhat prabinovRedhat force-pushed the fix_undefined_vars_postgresql branch from 363b268 to 0cb192f Compare September 12, 2024 11:30
Copy link
Contributor

@nirarg nirarg left a comment

Choose a reason for hiding this comment

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

This is look good
@p3ck can you please take a look again?

@nirarg nirarg merged commit 7710148 into redhat-cop:main Sep 12, 2024
17 checks passed
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.

3 participants