.../site-packages/salt/modules/ has priority over _modules #780

Closed
blast-hardcheese opened this Issue Feb 28, 2012 · 19 comments

Comments

Projects
None yet
5 participants
Member

blast-hardcheese commented Feb 28, 2012

Not sure if this is intentional, but it took me a while to figure out what was going on. Turns out I had an old version of my module in site-packages/salt/modules/, very frustrating.

Thoughts?

Member

SEJeff commented Feb 28, 2012

I would think that the system installed modules should override custom modules otherwise you could (potentially) run into issues when you override builtin salt modules accidentally and things start subtly breaking.

What do you think?

Owner

thatch45 commented Feb 28, 2012

If I recall my intentions, it was that the _modules overrode the system modules, I thought this was the behavior, but it seems that blast was running into problems here.

Regardless of what we choose, it needs to be clean, deterministic and documented

Member

blast-hardcheese commented Feb 29, 2012

I can see advantages on both sides, I think _modules should override the builtin modules, but maybe this could be a /etc/salt/minion configuration option? I think using system modules, states, etc over custom ones should be the default, but we should have a way to do that. It would make developing extensions to existing system modules /much/ easier.

Member

SEJeff commented Feb 29, 2012

Sounds reasonable to me.

Member

dcolish commented Mar 3, 2012

My votes for having _modules always override. I'm not sure about making this configurable since they're just python modules and there is already a good system for managing those.

Member

blast-hardcheese commented Mar 3, 2012

There's a possibility for confusion either way. Possibly some log message when a _module overrides a builtin?

On Mar 3, 2012, at 9:15, Dan Colish reply@reply.github.com wrote:

My votes for having _modules always override. I'm not sure about making this configurable since they're just python modules and there is already a good system for managing those.


Reply to this email directly or view it on GitHub:
#780 (comment)

Owner

thatch45 commented Mar 3, 2012

yes, _modules should allow you to override or add to an existing module, logging this event would not be a bad idea

Member

blast-hardcheese commented Mar 3, 2012

SEJeff, Before I try and make this happen, any thoughts?

Owner

thatch45 commented Mar 3, 2012

This should only require making sure the loader is bringing thing up in the right order, look at loader.py

Member

SEJeff commented Mar 3, 2012

@blast-hardcheese Consensus wins. So long as this behavior is enforced AND documented works for me. Salt is only as strong as it's community.

Member

blast-hardcheese commented Mar 5, 2012

I asked in channel, but just in case I'm AFK when someone sees it, I'm asking again here. I made the required modification to loader.py, but then noticed that render and grains could be refactored to also use _create_loader. Anyway, comments welcome: loader_restructure

Owner

thatch45 commented Mar 5, 2012

I like it blast! The only thing I would change is that we use .format to manage strings, but I like the modification and the fact that it unifies the loading interface, thanks!

Member

SEJeff commented Mar 5, 2012

@blast-hardcheese Nice! When you're done, would you mind unifying it all like you said?

Member

blast-hardcheese commented Mar 5, 2012

@thatch45: Changed string formatting to .format

@SEJeff: Sorry, memory might be faulty here; what are you referring to?

Owner

thatch45 commented Mar 5, 2012

Thanks blast, I am excited to get this pull req!

Member

SEJeff commented Mar 5, 2012

@blast-hardcheese disregard, I'm a bit loopy from working 12 hours yesterday and a 5-6 today for $real_job. Just a bit out of it I guess.

Member

blast-hardcheese commented Mar 5, 2012

@SEJeff No worries!

Closed out with f8d3bf8

This appears to still be happening for me on 0.15.1?

Owner

thatch45 commented May 22, 2013

Odd, you are sure the module is being distributed to the minion?

@jfindlay jfindlay added a commit to jfindlay/salt that referenced this issue Mar 23, 2016

@jfindlay jfindlay update suse master service patch
Fixes #780.
8de84b4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment