Refactor how files are watched #1355

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Member

nicklewis commented Dec 29, 2012

LoadedFile is now WatchedFile (since it has nothing to do with loading
files, only with telling when they change), and some of its internal
variables have been renamed for clarity. Also added a FileWatcher class
whose entire purpose is to track a set of LoadedFiles in aggregate. This
is now used by TypeCollection in place of its own hash.

Puppet::FileServing::Configuration::Parser and
Puppet::Util::NetworkDevice::Config were inheriting LoadedFile, simply
for its changed? method, even though neither of them really has file
properties. Now they wrap a WatchedFile instance instead.

Refactor how files are watched
LoadedFile is now WatchedFile (since it has nothing to do with loading
files, only with telling when they change), and some of its internal
variables have been renamed for clarity. Also added a FileWatcher class
whose entire purpose is to track a set of LoadedFiles in aggregate. This
is now used by TypeCollection in place of its own hash.

Puppet::FileServing::Configuration::Parser and
Puppet::Util::NetworkDevice::Config were inheriting LoadedFile, simply
for its changed? method, even though neither of them really has file
properties. Now they wrap a WatchedFile instance instead.

CLA Signed by nicklewis on 2010-08-23 21:00:00 -0700

Contributor

adrienthebo commented Apr 11, 2013

@nicklewis it looks like this updated the Racc parser along with the WatchedFile changes, and the parser code has been completely overhauled which is producing merge conflicts. Does the parser definition actually need to be changed? Is this PR still needed?

Member

nicklewis commented Apr 11, 2013

I haven't looked at the recent changes to the parser. The only change to the parser in this pull request is removing a require which wasn't needed anymore. If LoadedFile hasn't been refactored in master, then this change is likely still generally beneficial. I'll look into rebasing it.

Contributor

zaphod42 commented Apr 12, 2013

I have a strange feeling that this is being used by plugins people have written. I may be wrong on that and I would love to make it not public API, but we may be in a world right now in which this change would be breaking people.

Contributor

adrienthebo commented Apr 15, 2013

@zaphod42 is there a way to merge this into a branch that will only go into 4.0, should we just hold off on merging this, or should we close this and reopen when we're working on 4.0?

Contributor

jeffmccune commented Apr 15, 2013

@adrienthebo Changes that are breaking simply don't get merged under our current process. It's by design that we don't have a long-running branch that targets the next major release, primarily because long-lived branches have an upkeep cost for their entire life.

We can either leave this open and unmerged if it's it's a breaking change, or we can change it so that it's not a breaking change somehow. We can also close this and come back to it closer to the 4 release.

I don't think we need to add a new setting for every change in behavior we want to enable. An environment variable may be a suitable way to avoid the ever-growing number of settings while still getting the change in behavior into an opt-in state.

Contributor

jeffmccune commented Apr 25, 2013

@nicklewis @zaphod42 As far as I can tell this isn't a breaking change, I've never heard of an user or 3rd party developer that is relying on the internal LoadedFile class. Of course, that doesn't mean someone, somewhere, is doing so.

I'd like to merge this into master and then keep an eye out for someone filing an issue in Puppet 3.3 or later. I think the overall benefit of cleaning this up will apply to all users and the potential risk affects a miniscule (hopefully zero) number of users.

I've already rebased this in my local branch and the tests are all passing, including the new egrammar tests, so I'm going to go ahead and merge this in. I'd like to do this since the card has been sitting on our board for awhile and it's not clear what the next actions are. Let's revert it if anyone thinks my course of action is in error or if any problems come up. Sound OK?

-Jeff

Contributor

jeffmccune commented Apr 25, 2013

summary: Merged into master as fd1bc89. This should be released in Puppet 3.3. Thanks again for the contribution! Please note the following "best effort" actions we've taken regarding backwards compatibility; @nicklewis @zaphod42 As far as I can tell this isn't a breaking change, I've never heard of an user or 3rd party developer that is relying on the internal LoadedFile class. Of course, that doesn't mean someone, somewhere, is doing so. I'd like to merge this into master and then keep an eye out for someone filing an issue in Puppet 3.3 or later. I think the overall benefit of cleaning this up will apply to all users and the potential risk affects a miniscule (hopefully zero) number of users.

-Jeff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment