-
-
Notifications
You must be signed in to change notification settings - Fork 263
Make nodejs_generate_etc_profile an actual bool #171
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
base: master
Are you sure you want to change the base?
Conversation
|
As a work around, you can add an [defaults]
ALLOW_BROKEN_CONDITIONALS=True |
|
It would be wonderfull if this can be merged soon so we can make a use of this role again |
…odejs 🐛 This allows running the playbook with Ansible >= 2.19. related: geerlingguy/ansible-role-nodejs#171
…odejs 🐛 This allows running the playbook with Ansible >= 2.19. related: geerlingguy/ansible-role-nodejs#171
…odejs 🐛 This allows running the playbook with Ansible >= 2.19. related: geerlingguy/ansible-role-nodejs#171
tasks/main.yml
Outdated
| NPM_CONFIG_PREFIX: "{{ npm_config_prefix }}" | ||
| NODE_PATH: "{{ npm_config_prefix }}/lib/node_modules" | ||
| NPM_CONFIG_UNSAFE_PERM: "{{ npm_config_unsafe_perm }}" | ||
| NPM_CONFIG_UNSAFE_PERM: "{{ npm_config_unsafe_perm | bool }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to do the opposite of what should be done? This is coercing what is now (by default) a bool to a bool. Presumably you want to coerce a bool to a string instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coercing to string would cause the exact same issue we're trying to avoid here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line isn't a conditional. It's an environment variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, fixed
tasks/main.yml
Outdated
| npm: | ||
| path: "{{ nodejs_package_json_path }}" | ||
| when: nodejs_package_json_path is defined and nodejs_package_json_path | ||
| when: nodejs_package_json_path is defined and nodejs_package_json_path | bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should already be a bool too, I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be but doesn't have to, that's why we're coercing it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't and make it a bool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added parentheses to make the intention here clearer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh I totally misread this line, apologies. Seems like it would be better/clearer to check explicitly for an empty string? But, 🤷
Ansible 2.19 requires values used in conditionals to be actual bools and not just values that can be coerced to a bool