tl;dr I'll push the below change to ldap.rb if no objections.
> On Jan 10, 2018, at 8:50 AM, Sam Ruby <ru...@intertwingly.net> wrote:
>
> On Wed, Jan 10, 2018 at 11:27 AM, Craig Russell <apache....@gmail.com> wrote:
>> Thanks for the explanation. I now know what these two lines are supposed to
>> do, but apparently they don't work.
>>
>> I removed the (redundant) check from modify_pmcchairs for testing and:
>>
>> [MacBook-Pro-9:/srv/whimsy/tools] clr% ./modify_pmcchairs.rb --add clr
>> Password for clr:
>> [MacBook-Pro-9:/srv/whimsy/tools] clr% ./modify_pmcchairs.rb --add clr
>> Password for clr:
>> _WARN [#<LDAP::Mod:0x7f966a1238a8 LDAP_MOD_ADD
>> {"member"=>[]}>]
>>
>> IIUC this means that the
>> _WARN cn=pmc-chairs,ou=groups,ou=services,dc=apache,dc=org
>> /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:260:in `modify':
>> Protocol error (LDAP::ResultError)
>> from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:260:in
>> `modify'
>> from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:1377:in `add'
>> from ./modify_pmcchairs.rb:36:in `block in <main>'
>> from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:145:in `bind'
>> from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:145:in `bind'
>> from ./modify_pmcchairs.rb:36:in `<main>'
>> [MacBook-Pro-9:/srv/whimsy/tools] clr% ./modify_pmcchairs.rb --rm clr
>> Password for clr:
>> [MacBook-Pro-9:/srv/whimsy/tools] clr% ./modify_pmcchairs.rb --rm clr
>> Password for clr:
>> _WARN [#<LDAP::Mod:0x7fc03380b6f0 LDAP_MOD_DELETE
>> {"member"=>[]}>]
>> _WARN cn=pmc-chairs,ou=groups,ou=services,dc=apache,dc=org
>> /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:260:in `modify': Object
>> class violation (LDAP::ResultError)
>> from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:260:in
>> `modify'
>> from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:1368:in
>> `remove'
>> from ./modify_pmcchairs.rb:38:in `block in <main>'
>> from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:145:in `bind'
>> from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:145:in `bind'
>> from ./modify_pmcchairs.rb:38:in `<main>'
>>
>> Here's the fix:
>>
>> diff --git a/lib/whimsy/asf/ldap.rb b/lib/whimsy/asf/ldap.rb
>> index 6f2065b..9d1c9f7 100644
>> --- a/lib/whimsy/asf/ldap.rb
>> +++ b/lib/whimsy/asf/ldap.rb
>> @@ -1365,6 +1365,7 @@ module ASF
>> def remove(people)
>> @members = nil
>> people = (Array(people) & members).map(&:dn)
>> + return if people.empty?
>> ASF::LDAP.modify(self.dn, [ASF::Base.mod_delete('member', people)])
>> ensure
>> @members = nil
>> @@ -1374,6 +1375,7 @@ module ASF
>> def add(people)
>> @members = nil
>> people = (Array(people) - members).map(&:dn)
>> + if people.empty?
>> ASF::LDAP.modify(self.dn, [ASF::Base.mod_add('member', people)])
>> ensure
>> @members = nil
>>
>> I think for human experience it's bad to return without doing anything and
>> without giving the user information that it's expected that nothing was done.
>>
>> Now that I know how this is supposed to work, the issue is where the
>> checking should be done and how to report the issue to the user.
>>
>> The code in ldap.rb is used everywhere so it is probably not good to puts
>> messages but perhaps better to raise a meaningful error instead of Object
>> class exception.
>
> This quickly becomes a philosophical discussion.
The best kind.
The component with the most information should be the one to report the
situation. In this case, modify_pmcchairs knows everything and therefore should
be the component to tell the user whats up.
./modify_pmcchairs.rb --add clr
The following ids were not added because they were already in pmc-chairs: clr
./modify_pmcchairs.rb --rm clr roger
The following ids were not removed because they are still in
committee-info.txt: roger
The following ids were not removed because they are not in pmc-chairs: clr
>
> If I ask to remove Fred and Barney from a list, and only Fred is on
> the list, what should the code do?
>
> Looking to the builtin methods for Array may provide some insights:
>
> $ irb
> irb(main):001:0> x = ["Fred", "Wilma"]
> => ["Fred", "Wilma"]
> irb(main):002:0> x.delete('Fred')
> => "Fred"
> irb(main):003:0> x.delete('Barney')
> => nil
> irb(main):004:0> x
> => ["Wilma"]
> irb(main):005:0>
>
> Perhaps the right thing to do is to have the remove function return
> the list of people who were actually added/removed?
For modify_pmcchairs, it would not hurt but would not help because
modify_pmcchairs already knows everything. In order to report context-sensitive
information it needs to do more work.
I'll push the change to ldap.rb if no objections.
Craig
>
> - Sam Ruby
>
> P.S. Ruby also has a class named Set. Neither adding an object
> already in the set, nor removing an object which is not in the set is
> an error. These methods return back the modified set.
>
>
>
>
>> Craig
>>
>>> On Jan 9, 2018, at 8:13 PM, Sam Ruby <ru...@intertwingly.net> wrote:
>>>
>>> On Tue, Jan 9, 2018 at 6:11 PM, Craig Russell <apache....@gmail.com> wrote:
>>>>
>>>>> On Jan 8, 2018, at 11:55 PM, Sam Ruby <ru...@intertwingly.net> wrote:
>>>>>
>>>>> On Mon, Jan 8, 2018 at 11:34 PM, Craig Russell <apache....@gmail.com>
>>>>> wrote:
>>>>>>
>>>>>>> On Jan 8, 2018, at 7:32 PM, Sam Ruby <ru...@intertwingly.net> wrote:
>>>>>>>
>>>>>>> On Mon, Jan 8, 2018 at 7:07 PM, Craig Russell <apache....@gmail.com>
>>>>>>> wrote:
>>>>>>>> /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:260:in `modify':
>>>>>>>> Object class violation (LDAP::ResultError)
>>>>>>>>
>>>>>>>> And error reporting is not great. I guess more checking is needed but
>>>>>>>> Object class violation is not very informative.
>>>>>>>
>>>>>>> Oh, and AGREED!
>>>>>>>
>>>>>>> LDAP sucks. You want to add zero members: Object class violation.
>>>>>>> You want to add somebody who is already a member: Object class
>>>>>>> violation. You want to remove somebody who is not a member: Object
>>>>>>> class violation.
>>>>>>>
>>>>>>> That's why the action that caused the error is logged. In this case:
>>>>>>>
>>>>>>> LDAP_MOD_DELETE
>>>>>>> {"member"=>[]}>
>>>>>>> cn=pmc-chairs,ou=groups,ou=services,dc=apache,dc=org
>>>>>>>
>>>>>>> Here you are deleting nobody. That apparently is not allowed.
>>>>>>
>>>>>> Seems like there is some error checking that could be done in the
>>>>>> ldap.rb code.
>>>>>>
>>>>>> Here is the remove code from lib/whimsy/asf/ldap.rb :
>>>>>>
>>>>>> # remove people from this service in LDAP
>>>>>> def remove(people)
>>>>>> @members = nil
>>>>>> people = (Array(people) & members).map(&:dn)
>>>>>> ASF::LDAP.modify(self.dn, [ASF::Base.mod_delete('member', people)])
>>>>>> ensure
>>>>>> @members = nil
>>>>>> end
>>>>>>
>>>>>> Who wrote this code?
>>>>>
>>>>> That would be me.
>>>>>
>>>>>> Would it be possible for this code to check before calling
>>>>>> ASF::LDAP.modify that the member actually exists already?
>>>>>
>>>>> The line above it takes the intersection between the list of people
>>>>> passed to remove and the current set of members. So if a person who
>>>>> is not a member of the service is passed in, they will be silently
>>>>> removed from the set.
>>>>
>>>> Evidence suggests that the line and the corresponding line in Service.add
>>>> are not working as intended.
>>>>
>>>> Where does the variable members come from? Is it different from @members
>>>> that is set to nil just before trying to remove existing members?
>>>
>>> members is a method call. Essentially, self.members().
>>>
>>> https://whimsy.apache.org/docs/api/ASF/Service.html
>>>
>>> @members is the cached value for this function. Setting it to nil
>>> will force the members method to fetch new data from LDAP.
>>>
>>> - Sam Ruby
>>>
>>>>> I'm not sure how to refactor it, but there are add and remove methods
>>>>> for a number of LDAP objects: Committee, Project, etc.
>>>>
>>>> I'm not sure that a refactoring is needed, just testing to make it work as
>>>> intended.
>>>>
>>>> Craig
>>>>>
>>>>> - Sam Ruby
>>>>>
>>>>>> Craig
>>>>>>
>>>>>>> - Sam Ruby
>>>>>>>
>>>>>>> - Sam Ruby
>>>>>>
>>>>>> Craig L Russell
>>>>>> Secretary, Apache Software Foundation
>>>>>> c...@apache.org http://db.apache.org/jdo
>>>>
>>>> Craig L Russell
>>>> Secretary, Apache Software Foundation
>>>> c...@apache.org http://db.apache.org/jdo
>>>>
>>
>> Craig L Russell
>> Secretary, Apache Software Foundation
>> c...@apache.org http://db.apache.org/jdo
Craig L Russell
Secretary, Apache Software Foundation
c...@apache.org http://db.apache.org/jdo
