Set arity on all functions #176
Conversation
Use the function arity feature to remove a lot of boilerplate code from the functions.
Puppet 2.7 isn't supported anymore by stdlib, so move it to allowed failures.
| @@ -12,7 +12,6 @@ matrix: | |||
| allow_failures: | |||
| - rvm: 2.0.0 | |||
| - rvm: ruby-head | |||
| include: | |||
There was a problem hiding this comment.
Why is this necessary?
There was a problem hiding this comment.
This change makes tests fail on 2.7,but that's not really supported by
stdlib 4.x anyway
On 11 Sep 2013 14:15, "Adrien Thebo" notifications@github.com wrote:
In .travis.yml:
@@ -12,7 +12,6 @@ matrix:
allow_failures:
- rvm: 2.0.0
- rvm: ruby-head
- include:
Why is this necessary?
—
Reply to this email directly or view it on GitHubhttps://github.com//pull/176/files#r6301930
.
There was a problem hiding this comment.
@dalen Unfortunately we still need to do our best effort to make stdlib 4.x work with Puppet 2.7 even though we said we'd be breaking backwards compatibility with the 4 major release.
The reason for this best effort is that we don't currently have an effective way to make sure modules that are compatible with Puppet 2.7 don't accidentally bring in the latest version of stdlib which would break compatibility with Puppet 2.7.
For example, on a Puppet 2.7 system, if an end user installs module "foo" from the forge and "foo" specifies a generic (not locked to a major or minor version) dependency on stdlib, then the latest version of stdlib will be pulled in, which will be incompatible with Puppet 2.7.
Until we sort out a way to resolve dependencies taking into account the base version of Puppet, then we need to be extra-careful about breaking compatibility with Puppet 2.7 even though we technically said we already did so in stdlib 4.
Does this make sense?
-Jeff
There was a problem hiding this comment.
Right, but would it be acceptable to skip all the affected test cases on puppet 2.7?
There are some other test cases already that are only run on certain puppet versions.
The only reason we should skip test cases in the master branch of stdlib when on Puppet 2.7 is if the test itself doesn't apply to Puppet 2.7. e.g. the behavior being tested is totally disabled on that particular version. If the behavior being tested and specified does apply to Puppet 2.7 then we should still test it with 2.7 until we figure out how to resolve the dependency issue. These seem like valid tests for Puppet 2.7, or am I misunderstanding?
Yes, and in these situations the test itself should not apply at all to the skipped puppet version. If the test does apply then it shouldn't be skipped. |
|
Well, 2.7 doesn't support function arity, so testing that it throws an error whit wrong number of parameters doesn't work on puppet 2.7. On the other hand maybe those tests could just be deleted as that functionality is really tested in puppet itself. Here we are just testing that we have set the value correctly. |
|
@dalen I don't think we can merge this patch until we figure out how to prevent stdlib 4 from being installed into a system that can't support it. Removing all of the error handling is a big change in behavior for all of these functions when they execute in previous Puppet versions. We could merge this PR if there is no change in behavior for Puppet 2.7, but it seems quite tedious to special case everything... |
|
It doesn't really change the behaviour of the functions though. Just removes some error handling for puppet 2.7 users which I think is fine at this point. If you are still sticking with that really old version you can't really expect to get all the bells and whistles. |
|
@dalen Sorry, we can't merge this pull request as is. Removing the error handling is a change in behavior. The next steps forward on this pull request is to change the patch to preserve the error handling for Puppet 2.7 or not merge this until we have a way to control the version of stdlib that's brought in as an automatic dependency when installing modules. We'll leave this open for awhile, but if there aren't any updates in a couple of weeks we'll go ahead and close the PR. Closing it doesn't mean we're rejecting it, just that we can't merge it as-is at this time. |
|
The only way to preserve it on puppet 2.7 would be to implement the arity On 19 September 2013 23:35, Jeff McCune notifications@github.com wrote:
Erik Dalén |
|
So, now 2.7.x is properly EOL, how does that affect this? |
It doesn't, unfortunately. Puppet Enterprise is the real problem and there are still supported versions of Puppet Enterprise that contain Puppet 2.7. I don't know when the EOL for the most recent PE version with Puppet 2.7 will be, but @stahnma might be able to chime in there. The best path forward really does seem to be to get the Puppet module tool to prevent installation of incompatible modules. |
|
@jeffmccune I think the real issue here is that "policy" is out of sync with the documentation. The README claims that 4.x does not support puppet 2.7.x, yet the "policy" is not to merge patches that break 2.7.x. Due to that I'm sure there are a lot of 2.7.x installations out there that have a 4.x version of stdlib installed. That's going to cause a rather rude surprise if some future version of 4.x stops working where a prior minor release was functioning. I'd like to suggest that the documentation be changed to reflect 4.x supporting 2.7.x and that a new major version bump is made when 2.7.x support can in fact be dropped. An alternative solution would be update the README with a note to developers about the kinda/sorta/maybe/fishy/quasi support of 2.7.x. |
|
@jhoblitt I think you've identified one of the key problems. stdlib 4.x was meant to break backwards compatibility with Puppet 2.7.x but when we found it negatively affected users we decided to preserve backwards compatibility. This makes the 4.x series equivalent to a minor version of the 3.x series from a semantic versioning point of view. Would you mind submitting a pull request to update the README? If so, I'll try and do it as soon as possible, but I'm on-site working with a partner this week so it might be awhile. -Jeff |
|
Is there a master branch or 5.x branch that this could be merged to though? But this doesn't even really break 2.7.x compatibility, you just don't get all bells and whistles if you are still running 2.7.x. |
I don't think so. There's no 5.x because we can't release 5.x for the same reason we shouldn't have released 4.x. master is where new backwards compatible features are placed and stable is where bugfixes are placed.
It does break 2.7.x compatibility by removing the behavior of catching errors when run on 2.7.x. |
Without this patch there is a disconnect between the documentation in
the README and our decision to not merge pull requests into the 4.x
series that break compatibility with Puppet 2.7.x
For example:
@jeffmccune I think the real issue here is that "policy" is out of sync with
the documentation. The README claims that 4.x does not support puppet 2.7.x,
yet the "policy" is not to merge patches that break 2.7.x. Due to that I'm sure
there are a lot of 2.7.x installations out there that have a 4.x version of
stdlib installed. That's going to cause a rather rude surprise if some future
version of 4.x stops working where a prior minor release was functioning.
I'd like to suggest that the documentation be changed to reflect 4.x supporting
2.7.x and that a new major version bump is made when 2.7.x support can in fact
be dropped. An alternative solution would be update the README with a note to
developers about the kinda/sorta/maybe/fishy/quasi support of 2.7.x.
Please also see this discussion:
puppetlabs#176 (comment)
|
So basically this depends on the EOL date for puppet enterprise 2.x, right? |
|
According to http://puppetlabs.com/misc/puppet-enterprise-lifecycle PE 2 is either already out of support if we consider this problem under "Feature Support" or it'll be 2014-09-30 if we consider this "Maintenance/Security Support" @RColeman Could you provide a decision on if the removal of these error handlers would fall under Feature Support or Maintenance Support? |
|
@jeffmccune That looks good -- sorry for being slow and thank you for beating me to it! I once had (maybe more than once) a request not to use stdlib 4.x in one of my modules due to 2.7.x compatibility concerns, this change should help clear up some confusion in the community at large. I also wonder if resolution on this issue has been holding up a new minor release? @dalen I think it's a difficult call to make as to when to drop 2.7 support. EPEL6 only released 2.7 in the "stable" repo a few weeks ago. There's also at least a few "large installation" sites that are active in the puppet community still running 2.7. Unless a module installation tool appears which can discern module/puppet version compatibility and said tool can be used with 2.7, abandoning 2.7 support in stdlib might lead to fragmentation issues. I do completely share your frustration with 2.7 being a drag on development. If stdlib switched to the master/stable branch pattern the puppet and facter repos follow, it would add the hassle of having to merge up from stable to master. That may not be much of an issue since the rate of change in stdlib has been low lately but I'd be cautious about making that switch if a release based on the 5.x master branch might be years out. |
|
I know some people are still running puppet 2.7 but if they can't upgrade that in years, why would they even update their puppet modules? Or have the expectation that the latest and greatest will work on a really old EOL puppet release? And also if a updated module tool was released, how would that update get to the people that don't update their software? |
|
@jeffmccune happy to but could I have a bit of clarification first? Did Puppet 3.x introduce better facilities for validation and error handling that this patch aims to adopt at the cost of Puppet 2.7.x support? As Jeff pointed out, stdlib is in a tight spot. It's the single most used module on Forge which carries some extra sensitivity around breaking changes. Unfortunately, most depend on stdlib >= X instead of a specific major series or range between versions. If we break v4 for Puppet 2.7/PE 2.x users, it's so easy for them to upgrade into a bad state. The Forge team is working to introduce a new version of the Until then, I'm asking everyone to be very deliberate about breaking changes (Puppet wise) going into any Puppet Labs module. I wish we were further ahead but that's the state of things. |
Sure, Puppet 3.x has the ability to check function arguments before sending values to the function implementation. Erik added this functionality IIRC and it's usually referred to as arity checking or arity support. Puppet 2.7.x doesn't have this so each function explicitly checks the number of arguments passed and raises an error. This patch removes the explicit checks in the functions, which is effectively no change in behavior for Puppet 3.x but is a change in behavior (errors are no longer caught) for Puppet 2.7.x. If we merge this then a PE 2 customer might report a bug, "Puppet used to catch error X but no longer catches error X" This is the scenario we're trying to understand. |
More succinctly, the answer to your question is yes. |
|
@RColeman The motivation to change to arity checking isn't to enable new sanity checking so much as it allows the removal of a bunch of boilerplate code. |
|
If patches for Puppet 2.7.x are still accepted it would probably be trivial to backport the arity checking to it. But it feels odd adding features to something that is EOL even for security updates now AFAIK. |
|
Even if arity checking went into 2.7.24+, it wouldn't help legacy PE users. It would be a way forward for OSS users that can't upgrade to 3.x. Are there any installation statistics? My gut feeling is the only way to make a change like this and realistically continue to treat 2.7 as a first class citizen would be via monkey patching... I'm sure that would make us lots of new friends. I don't think dumping boilerplate is in and of itself a compelling reason to cripple 2.7 support but I think it's important for there to be a clear narrative around when 2.7 support can be dropped. Is it a line in the sand date? Based on the profile of the user base? Eg, when 2.7 drops to 15% of the user base support with be dropped. Will a 5.x/next branch be opened up? If so, when. etc. |
|
@jeffmccune Thanks for the info. I'll have to give this some more thought. I was operating under the assumption that with this patch, Puppet 2.7 users that were using this function would fail catalog compilation. |
|
@RColeman with this patch if 2.7 users pass too many arguments it will silently ignore the extra arguments instead of giving a error. If they pass too few they will still get some sort of error but likely a much less descriptive one. If they pass a valid number of arguments nothing will change. |
|
Once Puppet 2.7 (PE 2.8) is fully gone we can take something like this. Until then I'm just close this so that it isn't haunting us. Thanks for the contribution, @dalen. |
Use the function arity feature to remove a lot of boilerplate code from
the functions.