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.

Sign up for GitHub

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

Jump to bottom

Wide refactoring of master map and map file management #119

Merged
merged 7 commits into from
Apr 21, 2018
Merged

Wide refactoring of master map and map file management #119

merged 7 commits into from
Apr 21, 2018

Conversation

jcbollinger
Copy link
Contributor

Pull Request (PR) description

A broadly-scoped refactoring of the features for managing the master map and individual map files. There are two key thrusts to this refactoring:

  1. Decoupling management of master map entries for management of the map files to which they refer, and
  2. Removing opportunities for inconsistent specification of map file properties, such as are responsible for issues Executable maps cannot be built from multiple pieces #104 and Catalog compilation can fail when managing the same map file via multiple autofs::map resources #107.

To reduce -- but not eliminate! -- the backward incompatibilities produced by this refactoring, several parameters of autofs::mount are retained but deprecated and made ineffective, and the whole autofs::map defined type is deprecated, though it still works as much as it ever did. The new map file manage approach relies on two new defined types: autofs::mapfile and autofs::fs_mapping. The former is responsible for managing the overall properties and optionally some or all of the content of one map file. whereas the latter manages additional mappings in a map file. Thus, autofs::mapfile takes on responsibilities that used to belong some to autofs::mount and some to autofs::map; the rest of the responsibilities of those types stay with autofs::mount on one hand and are taken on by autofs::fs_mapping on the other.

This refactoring also takes the opportunity to implement more detailed map file structure, assuming the Sun map file format that was the only one for which the old version was willing to manage mapfile content anyway. This is done in a manner that handles typical cases cleanly, but does not foreclose the more complex and esoteric alternatives supported by that format.

It should be noted that this refactoring "fixes" issue #104 by removing support for directly managing executable maps. This is no big change, however, because it appears that the module never did handle executable maps correctly, issue 104 notwithstanding. It would set executable map file modes, but it would not configure the corresponding master map entries to treat such files as programs, so (as far as I can determine) the file mode was irrelevant.

This Pull Request (PR) fixes the following issues

Fixes #103
Fixes #104
Fixes #107

@jcbollinger
Factored map file management out of autofs::mount
19ffd33 
Performed a wide-reaching refactoring to relieve autofs::mount of the
responsibility of managing map files.  It now manages only entries in
the master map.  Created a new autofs::mapfile resource specific to
managing map files, as autofs::map has the wrong structure for this.
Created a new autofs::mapping resource to allow map file management to
be performed collaboratively via multiple resources, as autofs::map is
wrong for this, too.

To look at it a different way, this refactoring takes the space covered
by autofs::mount and autofs::map, and spreads it among a more targeted
autofs::mount, and new autofs::mapfile and autofs::mapping.  Autofs::map
is deprecated, but not removed.
@jcbollinger
Noted future removal of autofs::map
3b3120d 
Added a notation to the main README file, describing the deprecation and
planned future removal of the autofs::map resource type.
@jcbollinger
Assorted Puppet and Ruby style corrections
86cfc4e 
Implemented a variety of corrections to comply with the project's
Puppet and Ruby style rules.
@jcbollinger
Fixed issues revealed by CI testing
882171a 
Fixed some issues that were revealed by failing CI tests and by the
process of fixing those.  Primarily, ensured that the Concat resource
for the master map is always declared if any autofs::mount is declared,
directly or indirectly.  This is the original behavior, which had
inadvertently been changed.  Also tweaked the format of some tests
that used the 'match' matcher, and changed the formats of a couple of
resource titles.
@jcbollinger
Followup style tweaks
dfe771c 
A few of the previous style changes yielded new violations of
project style rules.  These violations are now fixed, too.
@jcbollinger
One more go at satisfying style rules
f4ce9f2 
Fixed another newly-introduced style validation.
@dhollinger dhollinger added backwards-incompatible needs-feedback Further information is requested labels Apr 15, 2018
dhollinger
dhollinger requested changes Apr 15, 2018
View reviewed changes
Copy link
Member

@dhollinger dhollinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see any README.md updates that would be needed added.

Also, see my comments about the deprecation notices.

Otherwise, it looks great to me.

manifests/mapfile.pp
if $autofs::reload_command {
Concat {
before => Service[$autofs::service_name],
notify => Exec['automount-reload'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not against using the Exec resource for the time being to ensure the automount-reload is done correctly, I do think that if we need to rely on an Exec resource, then it might be a good idea to investigate the potential need for a dedicated type/provider for the autofs module relating to maps.

Especially now that the Resource API has been released and the Exec resource is a hack that was put in place because the pre-Resource API types/providers code is not easy to implement.

Copy link
Contributor Author

@jcbollinger jcbollinger Apr 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to replacing use of an Exec with an appropriate application of the resource API. In fact, I'm right with you in preferring to avoid Execs. This particular use is is just drawn straight from the approach taken by the previous version, however, and if you want to change that then I suggest making it a separate issue.

manifests/mount.pp Outdated
Optional[Boolean] $execute = undef, # deprecated, no effect
Optional[Boolean] $mapfile_manage = undef, # deprecated, no effect
Optional[Variant[Array, String]] $mapcontents = undef, # deprecated, no effect
Optional[Boolean] $replace = undef # deprecated, no effect
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the parameter has no effect, is it truly deprecated or has it in reality already been removed?

I think it would be better in this case to just remove anything that no longer has an effect. As per SemVer standards, much of the work in this PR is backwards incompatible, so a major version release would be required anyway implying breaking changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working on that. As a related question, then, I suggest considering whether you want to retain autofs::map. This PR retains it unchanged (but now undocumented). If you have in mind to make a clean break in the next major version, however, then it would perhaps be appropriate to remove it altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I have pushed an update that removes the deprecated / ineffective parameters from autofs::mount, fixes a couple of resulting test failures, and documents those removals in README.md.

manifests/mount.pp Outdated
if $mapfile.is_a(Autofs::Mapentry) and $mapfile_manage {
fail("Parameter 'mapfile_manage' must be false for complicated 'mapfile' ${mapfile}")
if $direct =~ NotUndef {
deprecation('autofs::mount::direct', 'Parameter $autofs::mount::direct is deprecated and has no effect. Whether a map is direct is determined (by Autofs itself) by the mount point: specify $mount as \'/-\' for a direct map.')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment on Line 84

manifests/mount.pp Outdated
} else {
$contents = "${mount} ${options}\n"
if $execute =~ NotUndef {
deprecation('autofs::mount::execute', 'Parameter $autofs::mount::execute is deprecated and has no effect. For a map implemented as an executable program, manage the map file via a File or similar resource.')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment on Line 84

manifests/mount.pp Outdated
$mapperms = '0755'
$maptempl = 'autofs/auto.map.exec.erb'
if $mapfile_manage =~ NotUndef {
deprecation('autofs::mount::mapfile_manage', 'Parameter $autofs::mount::mapfile_manage is deprecated and has no effect. autofs::mount resources now manage only the master map, not map files')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment on Line 84

manifests/mount.pp Outdated
$maptempl = 'autofs/auto.map.erb'

if $mapcontents =~ NotUndef {
deprecation('autofs::mount::mapcontents', 'Parameter $autofs::mount::mapcontents is deprecated and has no effect. Manage map contents via autofs::mapfile and autofs::mapping resources.')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment on Line 84

manifests/mount.pp Outdated
}

if $replace =~ NotUndef {
deprecation('autofs::mount::replace', 'Parameter $autofs::mount::replace is deprecated and has no effect. Use autofs::mapfile and autofs::mapping resources, or other resources of your choice, to manage map files')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment on Line 84

@dhollinger dhollinger mentioned this pull request Apr 15, 2018
Closed
@jcbollinger
Copy link
Contributor Author

jcbollinger commented Apr 16, 2018 via email

@dhollinger
Copy link
Member

I somehow missed the README changes

@dhollinger
Copy link
Member

@jcbollinger Did you take a look at the other changes I requested?

@jcbollinger
Removed deprecated params from autofs::mount
d13bb5c 
The several parameters of autofs::mount that had been made ineffective
and and flagged with deprecation warnings are now removed altogether.
Most of these were related to map file management, which autofs::mount
no longer does.  The removals are documented in README.md, and they
required a few adjustments to test code and data.  Took the opportunity
to clean some unused cruft out of the test data.
@jcbollinger
Copy link
Contributor Author

jcbollinger commented Apr 17, 2018 •
edited
Loading

I thought I had already commented to this effect, but maybe I mistakenly attached that to the other PR. Apologies, then, if you get this twice, but I have committed changes that remove the deprecated autofs::mount parameters altogether and fix a couple of test failures that resulted.

The autofs::map defined type remains, deprecated and no longer documented in the README, but still as functional as ever it was. You might want to consider removing that for the next major version, too.

@dhollinger dhollinger merged commit d35ae63 into voxpupuli:master Apr 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhollinger dhollinger dhollinger approved these changes

@vinzent vinzent Awaiting requested review from vinzent

@traylenator traylenator Awaiting requested review from traylenator

@bastelfreak bastelfreak Awaiting requested review from bastelfreak

Assignees
No one assigned
Labels
backwards-incompatible needs-feedback Further information is requested
Projects
None yet
Milestone
No milestone
2 participants
@jcbollinger @dhollinger