On 11 January 2018 at 00:28, Craig Russell <apache....@gmail.com> wrote:
>
>> On Jan 10, 2018, at 4:07 PM, sebb <seb...@gmail.com> wrote:
>>
>> On 10 January 2018 at 16:27, 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?
>>
>> Is that supposed to be 'return if empty?'
>
> I'm so happy that you are following this thread. Ruby compiler caught this
> also. ;-)
I was not following in great detail.
It's just that the two changes looked similar but not identical.
A break in the pattern, if you like.
> Craig
>>
>>> 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.
>>>
>>> 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
>
