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

Extension suggests replacements of modules with internal implentation names #573

Closed
1 task done
C0rn3j opened this issue Jul 28, 2022 · 13 comments
Closed
1 task done
Labels
bug Something isn't working

Comments

@C0rn3j
Copy link

C0rn3j commented Jul 28, 2022

Sanity check

  • I certify that the redhat.ansible extension is in use and the language of the document in this bug report shows up as Ansible

Summary

Here's my main.yaml

- name: Clone/Update Guard git repo
  ansible.builtin.git:
    repo: 'https://github.com/thedevs-network/the-guard-bot.git'
    dest: /opt/the-guard-bot
    update: true

- name: Install npm
  community.general.pacman:
    name: npm
    state: present

- name: Install packages based on package.json.
  community.general.npm:
    path: /opt/the-guard-bot

running ansible-lint against it I get the following

% ansible-lint main.yaml                                                         
WARNING  Listing 1 violation(s) that are fatal
git-latest: Git checkouts must contain explicit version.
main.yaml:8 Task/Handler: Clone/Update Guard git repo

You can skip specific rules or tags by adding them to your configuration file:
# .config/ansible-lint.yml
warn_list:  # or 'skip_list' to silence them completely
  - git-latest  # Git checkouts must contain explicit version.

Finished with 1 failure(s), 0 warning(s) on 1 files.

That is fine.

However, in VSC, if I hover over community.general.pacman or community.general.npm, I get suggestions to replace them.

Use community.general.packaging.os.pacman instead.
Use community.general.packaging.language.npm instead.

The only place where I can find these mentioned is https://github.com/ansible-collections/community.general/blob/main/meta/runtime.yml#L1102

After talking with one of the community.general contributors on Matrix, it seems like it shouldn't be a thing and is a bug

felixfontein:
  actually doing that replacement would be a REALLY BAD idea.
  community.general.packaging.os.pacman is an implementation detail that can change even in a bugfix release and leave you with something that isn't working
  you should definitely file a bug report for the plugin

Extension version

0.11.10

VS Code version

1.69.2

Ansible Version

ansible [core 2.13.2]
  config file = /etc/ansible/ansible.cfg
  configured module search path = ['/home/c0rn3j/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python3.10/site-packages/ansible
  ansible collection location = /home/c0rn3j/.ansible/collections:/usr/share/ansible/collections
  executable location = /usr/bin/ansible
  python version = 3.10.5 (main, Jun  6 2022, 18:49:26) [GCC 12.1.0]
  jinja version = 3.1.2
  libyaml = True

OS / Environment

Arch Linux
ansible-lint 6.3.0-1

Relevant log output

No response

@C0rn3j C0rn3j added bug Something isn't working new labels Jul 28, 2022
@felixfontein
Copy link
Contributor

This is really problematic since these internal names should not be exposed to users, and must really not be used.

One problem with redirects is that it is totally unclear which redirects should be suggested to users and which ones should not. If the source is deprecated, or the redirect is deprecated, it definitely should. But if the redirect is not, it depends a lot. The redirect community.general.docker_container → community.docker.docker_container should be suggested, a redirect community.general.foo → community.general.bar as well. But a redirect community.general.foo → community.general.something.foo? This depends on the collection (in case of community.general: no).

Maybe we should add a new attribute private_redirect for meta/runtime.yml, like this:

plugin_routing:
  modules:
    aerospike_migrations:
      redirect: community.general.database.aerospike.aerospike_migrations
      private_redirect: true

ansible-core will ignore this (except the sanity tests), but the VSCode plugin could use this flag to not suggest this redirect.

@felixfontein
Copy link
Contributor

This is probably more a bug in https://github.com/ansible/ansible-language-server since all the code that does something is in there. @ssbarnea does it makes sense to create a new issue there, or should this issue be moved over to that repo, or something else?

@ssbarnea
Copy link
Member

@ganeshrn @priyamsahoo ^

@felixfontein
Copy link
Contributor

@ganeshrn @priyamsahoo any update on this? From my point of view this is a pretty critical bug that should be fixed as soon as possible.

@priyamsahoo
Copy link
Contributor

priyamsahoo commented Aug 16, 2022

Sorry for the delay.
We will discuss this and let you know the solution/ decision.

@ssbarnea ssbarnea removed the new label Aug 24, 2022
@Igorgro
Copy link

Igorgro commented Sep 29, 2022

@priyamsahoo Is there any new info? I've found that the modules which are affected by this issue also does not provide autocompletion:

image

@felixfontein
Copy link
Contributor

Some updated would be great indeed. This is a very serious bug that has been around for at least two months now.

@felixfontein
Copy link
Contributor

I created an ansible-lint rule which flags the invalid actions generated: ansible/ansible-lint#2572

@priyamsahoo
Copy link
Contributor

Thanks for creating a rule in lint.
We are still trying to work this out, as we are not sure if it is a bug or not. If it is, we need to have filters before providing redirects.

@felixfontein
Copy link
Contributor

@priyamsahoo by not treating this as a bug you are risking that your uses will suffer a lot when they have to undo all these suggestions.

@priyamsahoo
Copy link
Contributor

@felixfontein, I meant to say 'security bug'.
BTW, we are having a discussion on this, and this issue will not be closed until we come to a conclusion as to what must be done here.

@ganeshrn
Copy link
Member

There is a similar issue raised on ansible-language-server and a PR to fix it. Please refer ansible/ansible-language-server#449

@ganeshrn
Copy link
Member

ganeshrn commented Oct 11, 2022

Some updated would be great indeed. This is a very serious bug that has been around for at least two months now.

@felixfontein
Sorry for the delayed response. As you rightly pointed out in your earlier comment the code changes for this PR will be in ansible-language-server repository. However we can keep this issue open to track the extension release with the fix in ALS

ssbarnea pushed a commit that referenced this issue Mar 15, 2024
Point them to the ansible-language-server repo
ssbarnea pushed a commit that referenced this issue Mar 15, 2024
## v1.1.0

### Minor Changes

- Update yaml to 2.x (#566) @priyamsahoo
- Add variable auto-completion feature when cursor inside jinja inline brackets (#552) @priyamsahoo

### Bugfixes

- Get module route for FQCN with more than 3 elements (#538) @fredericgiquel
- Replace sphinx with mkdocs (#544) @ssbarnea
- Modify package version info in meta-data (#530) @priyamsahoo
- Fix intermittent EE test failures (#533) @ganeshrn
- Fix github issue links in docs (#573) @antdking
- Fix ansible lint config parsing (#577) @priyamsahoo
- Add env variable to remove color from command result stdout (#579) @priyamsahoo

Co-authored-by: Ansible DevTools <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

No branches or pull requests

6 participants