Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#52446 closed defect (fixed)

Buildbot failure emails go to too many recipients

Reported by: ryandesign (Ryan Schmidt) Owned by: macports-tickets@…
Priority: High Milestone:
Component: contrib Version:
Keywords: Cc: mojca (Mojca Miklavec), jmroot (Joshua Root), neverpanic (Clemens Lang), raimue (Rainer Müller), larryv (Lawrence Velázquez)
Port:

Description (last modified by ryandesign (Ryan Schmidt))

Each buildbot failure email is sent not only to the maintainers of the ports affected by this build failure, but also to the recipients of all previous build failure emails.

I've looked at the code for a long time and don't understand it. Obviously the list of interested users isn't being cleared at the beginning of each build. But I don't see where in the code anything gets added to the list of interested users. I see that we have a custom PortsMailNotifier class that inherits from MailNotifier, which is "same as original, but calls portMessageFormatter with access to interested_users". But I don't see where portWatcherMessageFormatter ever makes use of the interested_users argument its been passed.

Change History (11)

comment:1 Changed 4 years ago by ryandesign (Ryan Schmidt)

Description: modified (diff)
Priority: NormalHigh

comment:2 Changed 4 years ago by mojca (Mojca Miklavec)

I would guess that

class PortsMailNotifier(MailNotifier, object):
    def __init__(self, fromaddr, *args, **kwargs):
        self.interested_users = set()

should reset the list of recipients to an empty set, but I don't fully understand why it doesn't.

You could try one more thing, adding

interested_users = set()

somewhere on top of portWatcherMessageFormatter.

One thing I'm not sure about is when and how the committer gets added and I'm not sure how

def useLookup(self, build):
    ...
    return defer.gatherResults(dl)

works, so I cannot pretend I'm an expert.

Maybe we need to override / initialize some further variable of PortsMailNotifier.

comment:3 in reply to:  2 ; Changed 4 years ago by raimue (Rainer Müller)

There is only one instance of PortsMailNotifier and it is created in our master.cfg. Therefore the code in __init__ is only run once.

As I understand it, the interested_users attribute is only used to pass the list of users from the portMessageFormatter to useLookup as there is no direct way for interaction.

Maybe it is as simple as this:

Index: master.cfg
===================================================================
--- master.cfg	(revision 153357)
+++ master.cfg	(working copy)
@@ -682,6 +682,7 @@
 
     # same as original, but calls portMessageFormatter with access to interested_users
     def buildMessageDict(self, name, build, results):
+        self.interested_users = set()
         msgdict = self.portMessageFormatter(self.mode, name, build, results,
                                             self.master_status, self.interested_users)
         return msgdict

comment:4 in reply to:  3 ; Changed 4 years ago by larryv (Lawrence Velázquez)

This would avoid creating a new object repeatedly:

  • master.cfg

     
    682682
    683683    # same as original, but calls portMessageFormatter with access to interested_users
    684684    def buildMessageDict(self, name, build, results):
     685        self.interested_users.clear()
    685686        msgdict = self.portMessageFormatter(self.mode, name, build, results,
    686687                                            self.master_status, self.interested_users)
    687688        return msgdict
    688689
Last edited 4 years ago by larryv (Lawrence Velázquez) (previous) (diff)

comment:5 in reply to:  4 Changed 4 years ago by ryandesign (Ryan Schmidt)

Replying to larryv@…:

This would avoid creating a new object repeatedly:

Ok, let's try that.

comment:6 Changed 4 years ago by larryv (Lawrence Velázquez)

Cc: larryv@… added

Cc Me!

comment:7 Changed 4 years ago by ryandesign (Ryan Schmidt)

Resolution: fixed
Status: newclosed

This works, thanks. r153387

I don't understand how it works. buildMessageDict clears interested_users, passes interested_users to portMessageFormatter, which does not appear to use it in any way, and then somehow by the time useLookup is called, it's been filled with the right data?

comment:8 in reply to:  7 ; Changed 4 years ago by larryv (Lawrence Velázquez)

Replying to ryandesign@…:

I don't understand how it works. buildMessageDict clears interested_users, passes interested_users to portMessageFormatter, which does not appear to use it in any way, and then somehow by the time useLookup is called, it's been filled with the right data?

portMessageFormatter populates interested_users in line 750 or thereabouts. I don’t really understand why it happens in that function.

comment:9 Changed 4 years ago by ryandesign (Ryan Schmidt)

Dangit, I must have again been looking at my previous version of master.cfg where I had removed lines 750/751 for testing. Thanks.

comment:10 in reply to:  8 ; Changed 4 years ago by mojca (Mojca Miklavec)

Replying to larryv@…:

Replying to ryandesign@…:

I don't understand how it works. buildMessageDict clears interested_users, passes interested_users to portMessageFormatter, which does not appear to use it in any way, and then somehow by the time useLookup is called, it's been filled with the right data?

You can look at what MailNotifier does in the buildbot sources. The PortsMailNotifier only overrides some functions. The useLookup is called by one of the class functions.

buildMessageDict() initializes a field that was added by us. Then it "forwards" that field to portMessageFormatter(). Not that self.interested_users is an output of portMessageFormatter(), not input. Now that I look at it again I see that I could probably do it differently, for example return it from portWatcherMessageFormatter() as part of msgdict.

So portWatcherMessageFormatter() fills in the list of interested users. And then useLookup() (called by another function) uses that list to add actual recipients.

portMessageFormatter populates interested_users in line 750 or thereabouts. I don’t really understand why it happens in that function.

Because that's a place where we know them already. We could of course duplicate the code that calculates the recipients (we could recalculate the recipients inside useLookup()) or we could write another class to do it. You can check how that was done in the old setup. It's not nearly ideal to do it that way, I know, but I had to do something ...

Does the committer also get a message about broken build?

comment:11 in reply to:  10 Changed 4 years ago by raimue (Rainer Müller)

Replying to mojca@…:

Does the committer also get a message about broken build?

No. I got an email for a failing build, but the committer was not in To or Cc.

I think the list of committers could be obtained from build.getInterestedUsers() as in the old buildbot code.

Note: See TracTickets for help on using tickets.