morgangraphics / ansible-role-nvm

Installs NVM & Node.js on Debian/Ubuntu and RHEL/CentOS
MIT License
102 stars 29 forks source link

Install NVM task fails on Ansible 7/core 2.14 #35

Closed dandelany closed 1 year ago

dandelany commented 1 year ago

Hi @morgangraphics - thanks for this useful role!

Describe the bug When I try to run this role with Ansible 7/core 2.14.1 installed, it fails at the "Install NVM task" with the following error:

fatal: [myhostname.local]: FAILED! => {"changed": false, "changed_when_result": "The conditional check ''already installed' not in nvm_result.stdout' failed. The error was: error while evaluating conditional ('already installed' not in nvm_result.stdout): 'dict object' has no attribute 'stdout'. 'dict object' has no attribute 'stdout'", "msg": "Unsupported parameters for (ansible.legacy.command) module: warn. Supported parameters include: executable, stdin_add_newline, _uses_shell, creates, _raw_params, strip_empty_ends, removes, stdin, chdir, argv."}

I believe this is because the warn parameter was deprecated and removed in the latest Ansible version. If I comment out these lines in tasks.yml, the task runs correctly:

#    args:
#      warn: false

I'd be happy to submit a PR removing these lines if, however I'm not enough of an Ansible expert to determine if that needs to be replaced with something, or if there are any other changes required to support Ansible core 2.14.

Expected behavior The role should install NVM without throwing an error in the latest version of Ansible

To Reproduce

Shell [e.g. Bash, Dash, ksh, tcsh, zsh] zsh on control node, bash on managed node

Desktop (please complete the following information):

morgangraphics commented 1 year ago

I appreciate the offer to submit a fix, however, I can't just remove something. Just because you are using X & Y version doesn't mean that other people aren't stuck with or opting to use older versions that still need support. This particular part of the code is a little gnarly as there are some potential security issues with piping. I've has some similar issues with newer Ansible upgrades #32 recently and have had to do some version checking. If you're willing to take the work, I'd welcome a PR

dandelany commented 1 year ago

I can't just remove something. Just because you are using X & Y version doesn't mean that other people aren't stuck with or opting to use older versions that still need support

Understood, that's why I opened this ticket to ask what a proper implementation looks like. As mentioned I'm not an Ansible expert so it's not clear to me why the warn option was deprecated or why it was necessary here in the first place. Is your recommendation to pass this option conditionally based on the ansible_version similar to your comment in #32? That code uses a when option to decide whether an entire task should run, is there any similar way to just conditionally pass the warn argument, or would we need two different tasks with "opposite" when statements for new/old Ansible versions?

morgangraphics commented 1 year ago

Let me take a look and see what the options are.

qkdreyer commented 1 year ago

looking forward that fix too! I can't use packer build anymore

howardt12345 commented 1 year ago

Are there any updates on this issue? I'm currently running into this issue as well.

morgangraphics commented 1 year ago

My apologies, I was traveling for most of the past month and a half. I'm testing the fix, along with some other minor updates to the role. A fix should come this week or early next week.

morgangraphics commented 1 year ago

Made some updates and removed the thing that was bothering ya'll in the latest release.