search

rvm / rvm1-ansible

The official ansible RVM role to install and manage your Ruby versions.
MIT License
271 stars 136 forks source link

update tasks/rubies.yml - fix bool misuse #212

Closed danochoa closed 4 years ago

danochoa commented 4 years ago

This fixes issues introduced by https://github.com/rvm/rvm1-ansible/commit/deb1d3e6212d9ec6605510554bf268d18688513e as described by #209. The linked commit resolved warnings about bare variables in conditionals, but ended up breaking the conditional logic by misusing the bool filter. The bool filter returns true for vars set to ‘yes’, ‘on’, ‘1’, and ‘true’, and returns false otherwise, see https://docs.ansible.com/ansible/latest/user_guide/playbooks_conditionals.html.

pkuczynski commented 4 years ago

@lpaulmp what you think?

pkuczynski commented 4 years ago

@danochoa could you please solve the conflicts?

danochoa commented 4 years ago

@pkuczynski hold on, looks like I added a change that's not supposed to be in here

danochoa commented 4 years ago

@pkuczynski should be good to go

pkuczynski commented 4 years ago

Cool, thanks! One more question: in other places we are using is defined - wouldn't it be better here too? At least for consistency reasons...

danochoa commented 4 years ago

Probably a good idea. Updated for consistency. Thanks.

pkuczynski commented 4 years ago

@thbar is it good to go now?

pkuczynski commented 4 years ago

@danochoa could you also add an entry in the CHANGELOG, please?

thbar commented 4 years ago

@pkuczynski I just approved, I believe it's in good shape for merging!

In the future (I see that there is work ongoing on the testing infrastructure at #215), we will likely be able to add a test case for that, to ensure it doesn't regress.

Poke @rvm/ansible, ready to merge, but unsure who can do that at the moment (without the Travis build passing?)

gildegoma commented 4 years ago

@thbar Once #215 is merged, this PR (and many others) should no longer fail (the Travis CI failure reason is that the build tries to install latest Bundler version on Ruby 2.2, but Bundler 2.0+ requires Ruby >= 2.3)

thbar commented 4 years ago

@pkuczynski can you force merge this one (I can't on my side)? It is ready and having it into master would be useful immediately to a number of people (I will also close related issues afterwards).

pkuczynski commented 4 years ago

Sure! Done!