[discussion] refactor inconsistent module naming

[sjorge]

As mentioned by @damon-atkins in PR #30414

There are indeed lots of ways modules get named, below is a table of what we currently have and what there virtualname is.

filename	virtualname	loaded
bsd_shadow.py	shadow	os = %BSD%
darwin_pkgutil.py	darwin_pkgutil	os = MacOS
darwin_sysctl.py	sysctl	os = MacOS
deb_apache.py	apache	os_family = Debian
deb_postgres.py	postgres	which = pg_createcluster
debian_ip.py	ip	os_family = Debian
debian_service.py	service	os in ('Debian', 'Raspbian', 'Devuan')
freebsd_sysctl.py	sysctl	os = FreeBSD
freebsdjail.py	jail	os = FreeBSD
freebsdkmod.py	kmod	kernel = FreeBSD
freebsdpkg.py	pkg	os = FreeBSD and os_release > 10
freebsdports.py	ports	os = FreeBSD
freebsdservice.py	service	os = FreeBSD
gentoo_service.py	service	os = Gentoo
linux_acl.py	acl	which = getfacl
linux_ip.py	ip	which = ip and not os_family in ('Debian', 'RedHat')
linux_lvm.py	lvm	which = lvm
linux_sysctl.py	sysctl	kernel = Linux
mac_group.py	group	kernel = Darwin and os_releaseinfo > (10, 7)
mac_shadow.py	shadow	salt.utils.is_darwin
mac_softwareupdate.py	softwareupdate	salt.utils.is_darwin
mac_timezone.py	timezone	salt.utils.is_darwin
mac_user.py	user	kernel = Darwin and os_releaseinfo > (10, 7)
netbsd_sysctl.py	sysctl	os = NetBSD
netbsdservice.py	service	os = NetBSD and path /etc/rc.subr
openbsd_sysctl.py	sysctl	os = OpenBSD
openbsdpkg.py	pkg	os = OpenBSD
openbsdservice.py	service	os = OpenBSD and path /etc/rc.d/rc.subr and not path /usr/sbin/rcctl
openbsdrcctl.py	service	os = OpenBSD and path /usr/sbin/rcctl
osxdesktop.py	desktop	salt.utils.is_darwin
rh_ip.py	ip	os_family = RedHat
smartos_imgadm.py	imgadm	salt.utils.is_smartos_globalzone and which = imgadm
smartos_virt.py	virst	vmadm in modules
smartos_vmadm.py	vmadm	salt.utils.is_smartos_globalzone and which = vmadm
solaris_fmadm.py	fmadm	salt.utils.is_sunos() and which = fmdump and which = fmadm
solaris_group.py	group	kernel = SunOS
solaris_shadow.py	shadow	kernel = SunOS
solaris_system.py	system	salt.utils.is_sunos and which = shutdown
solaris_user.py	user	kernel = SunOS and HAS_PWD
solarisips.py	pkg	kernel = SunOS and kernelrelease > 5.10
solarispkg.py	pkg	kernel = SunOS and kernelrelease <= 5.10
win_autoruns.py	autoruns	salt.utils.is_windows
win_dacl.py	win_dacl	salt.utils.is_windows and HAS_WINDOWS_MODULES
win_disk.py	disk	salt.utils.is_windows
win_dns_client.py	win_dns_client	salt.utils.is_windows
win_dsc.py	dsc	salt.utils.is_windows
win_file.py	file	salt.utils.is_windows and HAS_WINDOWS_MODULES
win_firewall.py	firewall	salt.utils.is_windows
win_groupadd.py	group	salt.utils.is_windows and HAS_DEPENDENCIES
win_iis.py	win_iis	salt.utils.is_windows
win_ip.py	ip	salt.utils.is_windows
win_network.py	network	salt.utils.is_windows and HAS_DEPENDENCIES
win_ntp.py	ntp	salt.utils.is_windows
win_path.py	win_path	salt.utils.is_windows and HAS_WIN32
win_pkg.py	pkg	salt.utils.is_windows and HAS_DEPENDENCIES
win_powercfg.py	powercfg	os = Windows
win_repo.py	winrepo	salt.utils.is_windows
win_servermanager.py	win_servermanager	salt.utils.is_windows
win_service.py	service	salt.utils.is_windows
win_shadow.py	shadow	salt.utils.is_windows
win_status.py	status	salt.utils.is_windows and has_required_packages:
win_system.py	system	salt.utils.is_windows and HAS_WIN32NET_MODS
win_task.py	task	salt.utils.is_windows and HAS_DEPENDENCIES
win_timezone.py	timezone	salt.utils.is_windows and which = tzutil
win_update.py	update	salt.utils.is_windows and HAS_DEPENDENCIES
win_useradd.py	user	salt.utils.is_windows and HAS_WIN32NET_MODS
win_wua.py	win_wua	salt.utils.is_windows and HAS_DEPENDENCIES

^ the list probably not even complete, aptpkg, yumpkg,... come to mind

In have a proposal to clean this up, I will update this issues this weekend with a proposal to refactor this. Seeing this will be a massive change (not feature wise but layout wise) I think It's best there is a discussion up front so there is no wasted time doing stuff that won't get merged.

[sjorge]

I propose the following naming convention:

{virtualname}[_{variant}][_{os|os_family|kernel}].py

I also realized there is no proper platform grains. e.g.

bsd = os:openbsd, os:freebsd, ... linux = kernel:linux solaris = kernel:sunos(, kernel:illumos*) windows = kernel:windows

examples:

module	format	notes
gem.py	{virtualname}.py	gem works the same on all platforms, nothing special
user_linux.py	{virtualname}_{os}.py	linux systems have there own flavor
user_solaris.py	{virtualname}_{os}.py	solaris-like systems have there own flavor
user_bsd.py	{virtualname}_{os_variant}.py	bsd systems have there own flavor
ip_debian.py	{virtualname}_{os_family}.py	debian family linux systems do it like X
ip_redhat.py	{virtualname}_{os_family}.py	redhat family linux systems do it like Y
ip_win.py	{virtualname}_{os}.py	windows systems do it like X
vmadm_smartos.py	{virtualname}_{os}.py	vmadm is very specific to SmartOS
pkg_apt.py	{virtualname}_{variant}.py	some systems use apt to manage packages
pkg_yum.py	{virtualname}_{variant}.py	some systems use yum to manage packages
pkg_pkg_solaris.py	{virtualname}_{variant}_{os_family}.py	solaris systems use pkg to manage packages, tool is not compatible with openbsd pkg
pkg_pkg_openbsd.py	{virtualname}_{variant}_{os}.py	openbsd systems use pkg to manage packages, tool is not compatible with solaris pkg

Making the uniform will require a lot of refactoring (on it self not a big problem). I'm not sure how to maintain backwards compatibility though :-1:

• if they ever get around to replacing the uname SunOS part with illumOS

[sjorge]

concrete example for fixing the systems modules

old filename	new filename	virtualname	loaded
system.py	system_linux.py	systems	G@kernel:linux and which = shutdown
win_system.py	systems_windows.py	systems	G@os:windows and HAS_WIN32NET_MODS
solaris_system.py	system_solaris.py	systems	G@kernel:sunos and which = shutdown

Of course the doc refs should also reflect the new filenames.

The above refactor should be backwards compatible! However some others like for pkg would be nearly impossible to do without breaking backwards compat :(

[The-Loeki]

There's been work on module 'dirs', would that not a much cleaner refactor if it can be done properly?

system
  |- __init__.py
  |- linux.py
  |- windows.py
pkg
  |- __init__.py
  |- yum.py
  |- apt.py
etc.

Although for this layout to work, further improvements would seem to have to be made to the module dir stuff on at least the 2015.8 branch

[damon-atkins]

Yes "Dirs" like above would make it nicer, but comments in #30533 have indicated its not straight forward.

If stuff might break, could a warning be printed and then continue by calling the new module code. Maybe a tool to search yaml and optionally replace it.

[sjorge]

It's worse than that though, I little too to search files for X and print a warning line X has been replaced by Y should not be too hard.

I also like the dirs approach, something did occur to me too... if we rename all that stuff it will make back porting/forward porting less easy. This is definitely something to consider too.

I also suggest this does not happen withing the 2015.x release but if it should be in a release with the major (year) bumped.

[damon-atkins]

I would suggest the warning starts in 2015.x to say it will be removed in 2016.x Or both work in 2015.x

[The-Loeki]

Well the versioning scheme doesn't work that way, it'd be something like +X periodic table elements deprecation warning, so considering we're all waiting for Boron (5), the warning'd be like 'this module will dissapear in Salt Neon (10)' & just wait & see what version number Neon would actually get.

Also I'm not so sure about the porting problems; by far most stuff should already be using the virtual names (e.g. pkg rather than package_freebsd) through the LazyLoader, which would leave manual provider config stanzas; it'd be easy to temporarily wrap those stanza's through a compat dict

provider = {
'freebsdpkg': 'pkg.fbsd',
'yum_pkg': 'pkg.yum',
}.get(provider, provider)

or something like that :)

[jfindlay]

Thanks for the great discussion on this. I think that if users are already using modules through their __virtualname__ refactoring their real names should not be too much of a problem. @whiteinge and @thatch45 may have some further comments.

[sjorge]

@jfindlay: It will probably break the provides section of the minion config.

But yes if they use purely __virtualnames__ they should be OK for most. The windows ones would still have odd virtual names, but so be it.

e.g.

old filename	new filename	virtualname
win_servermanager.py	servermanager_windows.py	win_servermanager

But for those a deprecated in warning could be added, and a temperarly mapping of win_servermanager to servermanager ?

[jfindlay]

@sjorge, I think you mean #30519.

[sjorge]

Yeah you are right linked the wrong issue/pr.

[sjorge]

Shell we start with trying to clean this up a but by renaming a few like so: {virtualname}[_{variant}][_{os|os_family|kernel}].py or shall we go for the directory approach mentioned in #issuecomment-173944984? The later would probably need some work to the loader I'm guessing.

[The-Loeki]

IMHO {virtualname}[_{variant}][_{specific}].py system is eloquent & elegant. IMHO the directory layout would be even more eloquent & elegant. If it's decided to go with the latter (eventually), it'd be a shame to spend time on the former.

As you mention, the loader would need some work. and preserving backwards compatibility for that change (although there's a grand total of 1 module now in a subdir layout) might be more difficult. Personally I'd like to hear more from the Salt devs & their view on the (future of) the Loader/'sub'module system to see how feasible the idea is.

[jfindlay]

I know @thatch45 has had strong reasons not to nest execution modules, so we should check with him before proposing to rearchitect the structure.

[lorengordon]

@sjorge, what is the reasoning for Windows modules to have 'win_' prepended to the virtualname? I don't think that's necessary. I rather like the semantics of having the same module name across all OSes.

Or are you saying that some modules do that today, which would get a deprecation warning while they switch to the new style, and that the new style does away with such prepending?

[sjorge]

@lorengordon If you look at the first table, the windows modules do so now (sadly)

[lorengordon]

Right, some Windows modules do that now. I'd rather not formalize that, though, and have all Windows modules do that...

[sjorge]

Those prefixes should be dropped, so for a time we'd need to support both with win_ and without.

[lorengordon]

Right, ok, so the latter part of my first comment. That works for me. Just trying to confirm that the direction of intent is to deprecate the 'win_' semantics for virtualnames.

[basepi]

I do not anticipate the directory path going anywhere in the near future. It would be a much more disruptive change.

I really like the {virtualname}[_{variant}][_{os|os_family|kernel}].py proposed solution. I think it's clean and consistent, and way easier to see which modules provide for the same virtualname, and what that virtualname is.

We can create a map of old names and compare with a warn_until for the provides configuration. Otherwise I don't think anything else should break (in theory) since we use virtualname.

I will note that backporting and merging forward will be made more difficult as a result of this change, but I think it will be worth it in the long run.

[damon-atkins]

@twangboy is renaming some mac modules

[sjorge]

I'll give it a shot this weekend to see if I can get one of the easier ones (system) to match: {virtualname}[_{variant}][_{os|os_family|kernel}].py, it should not need odd virtual mappings.

[damon-atkins]

It would be easy (i assume) to add a test that checks file names are prefixed with the virtual name. It also makes it easy to rename if it needs to be os/platform specific at a latter date.

[sjorge]

@damon-atkins where would that check be added? a githook?

[damon-atkins]

I am assuming Jenkins would do it as part of salts internal testing

[sjorge]

Oh yeah Jenkins would make a lot of sense

[damon-atkins]

Be aware of mac renames #30556 #30555 #30558 #30557 #30551 #30552

[damon-atkins]

So in Jenkins you someone would add an allow pattern... if a new os, platform, etc came along it would be add to the allow list. This way the naming standard is inforced.

[basepi]

The mac renames are all new, unreleased modules anyway.

[sjorge]

Cool, so not to much worrying about that. If the PR for the system module looks good I will see if I can do some others during my lunch breaks :)

[basepi]

Jenkins should be great for this, we can probably add it to the linter along with our custom checks like cli examples. (I assume we can, I'm actually not that familiar with our linting setup)

[thatch45]

Sorry this took me so long to catch up on, my weekend was insane.

1. Do not do dirs, the loader does handle it better now but it would be a real problem for user distributed modules
2. I will completely agree with the proposed file name scheme, it is good.
3. We need to allow the loaded to be able to abstract multiple virtualnames for deprecation paths to work, so that we can fix the bad windows virtualnames like win_dns_client.
4. name changes should be deprecated until the Nitrogen release (which should be around 2017.3 ish timeframe)

Did I miss anything?

[sjorge]

@thatch45 don't think you missed anything. I poked around a bit for item 3. but some of the dirty guts of saltstack are still not my friend. I think @basepi was going to look at this later this week.

[thatch45]

Oh, I think I might have to do item 3, this is adding a dunder keyword to the loader or adding list support to the virtualname, which might be hairy, so I will look at that first.

[sjorge]

Ack, ping me here once you have something. I have a PR for system open that is blocked on this (as providers would break without this)

[thatch45]

I may have missed something, provides map to filenames, item 3 in my list is to allow multiple virtualnames. I am not quite sure on how to solve the provider filename change, I will need to spend more time on that.

[damon-atkins]

Off topic a bit. But is their a reason for grains/core.py being one large large and growing file (1800 lines) to load, vs more like modules. E.g core_posix.py which calls

• core_solaris.py
• core_linux.py
• core_hpux.py core-windows If it's worthy of a discussion please start new one. I'm ignoring that i think grains should be prefixed with os, hw, net, fs, As thats a huge change.

[sjorge]

I also find the one big core.py file for grains uneasy to manage. But I work in that way less than I do in modules. I wouldn't seeing this refactored.

[thatch45]

Yes, please do not try to refactor grains modules.

[basepi]

@thatch45 My thought for provider overrides was just to manually create a map of old to new filenames. If one of the old filenames shows up in the provider overrides, we just replace it with the new filename, and throw a warn_until.

[basepi]

Do we need multiple virtualnames? None of the virtualnames will be changing, right?

[thatch45]

@basepi that plan for provides works, good thinking.

As for changing virtualnames, I am sure we will want to change some eventually, so we should look into adding the ability to do so

[sjorge]

We should probably drop the win_ prefix while we are at it

~ sjorge

On 27 Jan 2016, at 22:27, Thomas S Hatch notifications@github.com wrote:

@basepi that plan for provides works, good thinking.

As for changing virtualnames, I am sure we will want to change some eventually, so we should look into adding the ability to do so

— Reply to this email directly or view it on GitHub.

[whiteinge]

Please involve @jacobhammons here.

• All the documentation TOC will need to be recreated.
• All of the generated doc files will need to be regenerated.
• Cross-linked docs that make use of the :doc: role will need to be changed to point at the new path. (There shouldn't be many of these pointing at modules.)
• All the old bookmarked/index URLs will 404 after this change so 301 redirects would be desirable though not easy to put in place since it's a static site. (JavaScript/HTML redirects could be added to the old docs but those won't update search engines.)

[twangboy]

I would like to keep (win/mac) somewhere in the filenames. I agree they should be removed from the virtual name. We need to standardize our file naming.

[sjorge]

@twangboy I'm not sure what you mean by keep the win/mac in the filename

win_system.py -> system_windows.py mac_system.py -> system_darwin.py

As per the above proposed new naming convention. Both would have a virtual name of 'system'.

(darwin could probably be replaced with mac though)

[twangboy]

@sjorge I don't know how many times I've written a new module only to find someone has already done it. The file names would be for convenience for dumb guys like me. If everything started with mac_ I could see all the mac modules together and see what's missing. If they all ended with mac, I could see that there's not a mac module for such and such a command. As it stands now, they're all just mixed together with no distinction in the filenames as to which OS the module applies.

[damon-atkins]

"Darwin" might be a bit to generic https://en.wikipedia.org/wiki/Darwin_(operating_system)#Hardware_and_software_support ??? e.g. iPhone 5+, iPad Air, iPod, Apple TV & Mac's See also https://en.wikipedia.org/wiki/Uname (includes a table of results)

[sjorge]

@damon-atkins: sure we could settle on mac instead of darwin, I went for darwin in my propasal because salt.utils.is_darwin() is often used to check if we are on Mac OS X or not.

@twangboy: how would it be different with the propose name change? You can still see at a glance if something will work or not with the new naming, if you want know if system would be available on mac os x. You can check if there is a file called system_mac.py if there is none you can have a quick check of system.py to see if it is in the generic one.

If you want to inplement say a dnsmasq module for mac, you can check if there is one called dnsmasq.py (which is probably OS agnostic) or dnsmasq_mac.py (for the odd case it would be vastly different on mac)

Worst case a ls *_mac.py will give you a quick overview.

[twangboy]

@sjorge I don't care what the format is for the file name as long as there's some indication that a module exists for a specific OS.