Accept depth as option for Recursive membership validator strategy constructor

[mtodd]

This makes the GitHub::Ldap::MembershipValidators::Recursive#perform method signature identical to the other strategies, moving the options to the constructor to match the GitHub::Ldap::MemberSearch strategies too.

Technically this is a backwards incompatible API breakage but I don't think it's a significant enough change as to cause a major version bump since it's really fixing a bug with the API. Thoughts?

cc @jch

[mtodd]

Could make these minor changes to prevent the API breakage:

diff --git a/lib/github/ldap/membership_validators/recursive.rb b/lib/github/ldap/membership_validators/recursive.rb
index 8544f30..dfffdf4 100644
--- a/lib/github/ldap/membership_validators/recursive.rb
+++ b/lib/github/ldap/membership_validators/recursive.rb
@@ -36,7 +36,7 @@ module GitHub
           @depth = options[:depth] || DEFAULT_MAX_DEPTH
         end

-        def perform(entry)
+        def perform(entry, depth_override = nil)
           # short circuit validation if there are no groups to check against
           return true if groups.empty?

@@ -51,7 +51,7 @@ module GitHub
             next if membership.empty?

             # recurse to at most `depth`
-            depth.times do |n|
+            (depth_override || depth).times do |n|
               # find groups whose members include membership groups
               membership = domain.search(filter: membership_filter(membership), attributes: ATTRS)

But I think that's being pedantic and the practical side of me just wants to merge this and cut 1.7.0.

[jch]

I would document the depth attribute here.

[jch]

@mtodd since we are at a > 1.0, I'd like to maintain semver. Even though we're likely the primary users of this library, it'll make it easier on ourselves if we ever need to downgrade our own library for an important fix. I would also output to stderr with a deprecation notice if someone calls the old interface.

[mtodd]

@jch documented depth option.

[mtodd]

it'll make it easier on ourselves if we ever need to downgrade our own library for an important fix

Generally I agree with you. But this part is more theoretical than actual since we never used the now-deprecated API at any time, so at the very least, assuming we're the only user of this component, we've solved zero problems going out of our way to follow a spec.

In any case, I've kept the current API, added deprecation warning, and will release this as 1.7.0. I'll remove the relevant bits for 2.0.

[jch]

Generally I agree with you. But this part is more theoretical than actual since we never used the now-deprecated API at any time, so at the very least, assuming we're the only user of this component, we've solved zero problems going out of our way to follow a spec.

I hear your frustration that it feels like extra work for no gain in this instance. But I think we ended up having to change the public interface so quickly because we hadn't used it enough ourselves to understand what the correct public interface should look like. To avoid this extra work maintaining semver in the future, we can experiment with changes in our own project first before pushing the changes upstream.