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

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

Closed
dandelany opened this issue Dec 11, 2022 · 7 comments
Closed

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

dandelany opened this issue Dec 11, 2022 · 7 comments
Assignees

Comments

@dandelany
Copy link

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

  • Install the latest version of ansible
  • Clone this repo into roles
  • Make a playbook:
- name: test the thing
  hosts: homelab
  roles:
    - role: ansible-role-nvm
      nodejs_version: "lts/gallium" # Node 16

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

Desktop (please complete the following information):

  • OS: OSX
  • Ansible Version: Ansible Community 7 / core 2.14.1
@morgangraphics
Copy link
Owner

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
Copy link
Author

dandelany commented Dec 14, 2022

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
Copy link
Owner

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

@qkdreyer
Copy link

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

@howardt12345
Copy link

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

@morgangraphics
Copy link
Owner

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants