saltuitl.sync_modules is not a sync it is a copy

[torhve]

I tried running sync_modules to get rid of an old module that I had used to override a salt module using the same name. The saltutil.sync_modules command happily syncs changes in existing files and new files, however it does not delete files that should not be there.

In my opinion, either the documentation should mention this, since a sync would mean to bring the directory exactly in line with what the salt fileserver has in that directory, or preferrably the sync operation should actually delete old/missing files.

[holmboe]

I concur. And if sync_modules does not handle removal of modules then there should be another function to delete them. I have resorted to manually deleting files in the filesystem which is a bit cumbersome.

[basepi]

Thanks for pointing this out. Unloading modules is currently not very well-supported, and we need to implement it.

[cachedout]

Adjusted the docs.

@basepi Should we perhaps rename this issue and file it as a feature to delete stale modules for the Hydrogen release?

[thatch45]

When I originally wrote sync_modules it would delete the old ones, or at least I remember it doing so....

[cachedout]

...researching...

[cachedout]

@thatch45 is right. This code definitely exists. I will test it post-haste to ensure it's still working.

[cachedout]

An update on this.

I spent some amount of time diving into this issue yesterday. In my testing, the sync process did correctly delete the modules on the remote filesystem. However, the modules were not being unloaded from the list of minion functions stored in salt even after saltutil.refresh_modules was run and the correct events were fired and picked up on the minion.

Further complicating this issue is the fact that running salt-call sys.list_modules on the minion does report that the module has been unloaded. It's only from the view of the master that the module does not appear to be unloaded.

We'll keep looking at this issue.

[cachedout]

I need @thatch45 to review the above change before it gets merged in but it's addressed the module unloading bug thus far in my testing.

[cachedout]

The above PR should resolve the issue. If for some reason this is not the case, please feel free to comment and we'll happily re-open it.