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

Avoid jinja2 in when warnings #3

Closed
wants to merge 1 commit into from
Closed

Avoid jinja2 in when warnings #3

wants to merge 1 commit into from

Conversation

major
Copy link

@major major commented Oct 10, 2017

This patch adjusts the handler to use the jinja2 statement directly
rather than using a variable with delimiters. This will hopefully
avoid the warning we currently see in OpenStack-Ansible:

[WARNING]: when statements should not include jinja2 templating
delimiters such as {{ }} or {% %}.
Found: {{ groups['haproxy'] | default([]) | length > 0 }}

Copy link
Owner

@logan2211 logan2211 left a comment

Choose a reason for hiding this comment

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

Good catch.. Small suggestion to maintain the override ability to null out the handler if desired.

@@ -13,9 +14,6 @@
# See the License for the specific language governing permissions and
# limitations under the License.

# Can be used to completely disable the role's handler actions
haproxy_management: "{{ haproxy_hosts | default([]) | length > 0 }}"
Copy link
Owner

Choose a reason for hiding this comment

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

Let's leave this in and default it to yes so there is still a way for someone to completely disable the role actions even if haproxy is present.

Copy link
Owner

Choose a reason for hiding this comment

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

Actually I wonder if it might work to simply remove the jinja brackets from this var default and set it to:

haproxy_management: 'haproxy_hosts | default([]) | length > 0'

with no other changes

Copy link
Author

Choose a reason for hiding this comment

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

You'd still need that jinja2 to be evaluated, though.

@@ -26,5 +27,5 @@
weight: "{{ haproxy_weight | default(omit) }}"
delegate_to: "{{ item }}"
with_items: "{{ haproxy_hosts }}"
when: haproxy_management | bool
when: haproxy_hosts | default([]) | length > 0
Copy link
Owner

Choose a reason for hiding this comment

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

following on the above comment id suggest doing this:

when:
  - haproxy_hosts | default([]) | length > 0
  - haproxy_management | bool

This patch adjusts the handler to use the jinja2 statement directly
rather than using a variable with delimiters. This will hopefully
avoid the warning we currently see in OpenStack-Ansible:

    [WARNING]: when statements should not include jinja2 templating
    delimiters such as {{ }} or {% %}.
    Found: {{ groups['haproxy'] | default([]) | length > 0 }}
@major
Copy link
Author

major commented Oct 13, 2017

I tried another strategy. We do a check to see if there are haproxy hosts and we also check to see if haproxy_management has been set to no. Does that make sense?

Copy link
Owner

@logan2211 logan2211 left a comment

Choose a reason for hiding this comment

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

Yep LGTM that was my first suggestion exactly :D

Only one small nit..

@@ -14,7 +15,7 @@
# limitations under the License.

# Can be used to completely disable the role's handler actions
haproxy_management: "{{ haproxy_hosts | default([]) | length > 0 }}"
haproxy_management: yes
Copy link
Owner

Choose a reason for hiding this comment

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

Can you trim the whitespace at the end of the line

@major major closed this Apr 29, 2020
@major major deleted the avoid-jinja-warning branch April 29, 2020 14:43
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.

None yet

2 participants