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

Extmods should override the internal salt module even with different filename #52521

Closed
TeddyAndrieux opened this issue Apr 12, 2019 · 6 comments
Closed
Labels
Bug Documentation P4 severity-medium stale
Milestone

Comments

@TeddyAndrieux
Copy link
Contributor

@TeddyAndrieux TeddyAndrieux commented Apr 12, 2019

Description of Issue/Question

Currently when we define an extmods with __virtual__ returning the same name as an already existing salt internal module that's only merge new function and not override the already existing one.

Steps to Reproduce Issue

Created a simple extmodule to override test

[root@bootstrap ~]# tree salt/
salt/
└── _modules
    └── testmod.py

1 directory, 1 file
[root@bootstrap ~]# cat salt/_modules/testmod.py 
__virtualname__ = "test"

def __virtual__():
    return __virtualname__

def echo(*args, **kwargs):
    return "custom module"

def echov2(*args, **kwargs):
    return echo(*args, **kwargs)

I sync salt modules and try to execute the 2 function and echo does not get overwrited

[root@bootstrap ~]# salt-call --local --file-root /root/salt test.echo "test"
local:
    test
[root@bootstrap ~]# salt-call --local --file-root /root/salt test.echov2 "test"
local:
    custom module

Renaming the python file with the same name as the one from salt internal really override the whole module

[root@bootstrap ~]# mv /root/salt/_modules/test{mod,}.py
[root@bootstrap ~]# salt-call --local --file-root /root/salt saltutil.sync_all
local:
    ----------
    beacons:
    clouds:
    engines:
    grains:
    log_handlers:
    modules:
        - modules.test
    output:
    pillar:
    proxymodules:
    renderers:
    returners:
    sdb:
    states:
    thorium:
    utils:
[root@bootstrap ~]# salt-call --local --file-root /root/salt test.echo "test"
local:
    custom module

Versions Report

Salt Version:
           Salt: 2018.3.4
 
Dependency Versions:
           cffi: 1.6.0
       cherrypy: Not Installed
       dateutil: 1.5
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.7.2
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: 0.31.0
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: Not Installed
      pycparser: 2.14
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.5 (default, Oct 30 2018, 23:45:53)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 15.3.0
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.1.4
 
System Versions:
           dist: centos 7.6.1810 Core
         locale: UTF-8
        machine: x86_64
        release: 3.10.0-957.1.3.el7.x86_64
         system: Linux
        version: CentOS Linux 7.6.1810 Core
@Ch3LL
Copy link
Contributor

@Ch3LL Ch3LL commented Apr 15, 2019

looks like i'm able to replicate this behavior all the way back to the 2017.7 branch. But in our docs (https://docs.saltstack.com/en/latest/ref/modules/#virtual-function) it states :

Modules which return a string from __virtual__ that is already used by a module that ships with Salt will override the stock module.

so it seems we need to make sure this behavior is added in. thanks

@Ch3LL Ch3LL added Bug severity-medium P4 triage labels Apr 15, 2019
@Ch3LL Ch3LL added this to the Approved milestone Apr 15, 2019
@max-arnold
Copy link
Contributor

@max-arnold max-arnold commented Nov 15, 2019

I believe this is intended behavior, and the docs are slightly misleading. There is a couple of related comments in the loader:

salt/salt/loader.py

Lines 1699 to 1700 in 961d192

# If we had another module by the same virtual name, we should put any
# new functions under the existing dictionary.

# Careful not to overwrite existing (higher priority) functions

To achieve what you want (override just one existing function), you can use a hack from salt-jenkins repo. A few examples:

https://github.com/saltstack/salt-jenkins/blob/develop/_modules/pip.py
https://github.com/saltstack/salt-jenkins/blob/develop/_modules/win_pkg.py
https://github.com/saltstack/salt-jenkins/blob/develop/_modules/winrepo_pkg.py

@max-arnold
Copy link
Contributor

@max-arnold max-arnold commented Nov 15, 2019

@Ch3LL
Copy link
Contributor

@Ch3LL Ch3LL commented Dec 12, 2019

looks like your right @max-arnold

ping @saltstack/team-core i assume we want to keep this behavior? any thoughts?

@Ch3LL Ch3LL added Documentation and removed triage labels Dec 12, 2019
@stale
Copy link

@stale stale bot commented Jan 11, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

@stale stale bot added the stale label Jan 11, 2020
@stale stale bot closed this as completed Jan 19, 2020
@md-magenta
Copy link
Contributor

@md-magenta md-magenta commented Mar 24, 2021

The documentation have not been updated and this should be reopened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Documentation P4 severity-medium stale
Projects
None yet
Development

No branches or pull requests

4 participants