CDSK-873 - remove builds by 'brid'. Add logging and tests

[warcholprzemo]

1. remove dict from unclaimedBrdicts / resumeBrdicts by 'brid' key instead full dict
2. Log if the method does not remove anything from first or second list
3. Add tests for removeBuildRequest

[coveralls]

Coverage increased (+0.01%) to 68.544% when pulling debf674 on CDSK-873-upgrade-KatanaBuildChooser-removeBuildRequests into 460306a on staging.

[campos-ddc]

That looks more complicated than necessary :)

I thought about something like this (unless you see something that wouldn't work):

def removeBuildRequest(self, breq):
    # reset the checkMerges in case the breq still in the master cache
    def remove_dict(brdicts):
        for brdict in brdicts:
            if brdict['brid'] == breq.brdict['brid']:
                brdicts.remove(brdict)
                return True
        return False

    if not breq.brdict:
        pass # log some error here
    
    removed = remove_dict(self.unclaimedBrdicts) or remove_dict(self.resumeBrdicts)
    if removed:
        breq.checkMerges = True
        breq.retries = 0
        breq.hasBeenMerged = False
        self.breqCache.remove(breq.id)
    else:
        klog.err_json(
            "removeBuildRequest does not remove anything. breq.brid: %s" % \
            breq.brdict['brid']
        )

[warcholprzemo]

list comprehension removes all dicts with the ['brid'] so we don't leave a possible mess in memory. In your case we remove only one dict from list. @Mowinski What do you think?

[campos-ddc]

They shouldn't show up multiple times in there, but you can do it like this to feel safer:

def remove_dict(brdicts):
    removed = False
    for brdict in brdicts:
        if brdict['brid'] == breq.brdict['brid']:
            brdicts.remove(brdict)
            removed = True
    return removed

[campos-ddc]

I'm not a huge fan of replacing the entire list with a comprehension because I don't know if anyone else has a hard reference to the old list.

[Mowinski]

As I said below, I am not a fan of inner function in this place :) and I agree with Diogo, whats more, as I see we modify the property of class so even we do not need an inner function to do it and a private method should be enough to do it.

[warcholprzemo]

OK, I've used Diogo's code

[Mowinski]

I think that using an inner function in methods is a little bit unpractical. There are three cases when we should use the inner function:

1. To separate assertion/parameters checking from recursive code
2. To encapsulate function, if there is a reason to hide it from the global scope
3. To create factory function, that using closure.

removeBuildRequest isn't recursive function so the first point doesn't affect it.
This is also a method, so if you want to make it private, you can create remove_dict as a private method in the class.
And the last point, this method isn't factory, so using closure variable is an unwanted side effect.

So in my opinion, you can create remove_dict to a static method or a utils function and do not worry about scope :)

Here is link to great article about inner function: https://realpython.com/inner-functions-what-are-they-good-for/

[Mowinski]

Of course, it would be nice to add docstring and a little bit change name of this function, now it suggests that from brdicts remove dictionary. Ensure that u want to modify parameters or just return a copy.

[Mowinski]

minor: this msg can be in if statement.

[Mowinski]

In this case, you do not need mock, you can use simple stub

BregClass = namedtuple('Breg', ['brdict'])
breq = BregClass(brdict={'brid': 3})

But please be aware and ensure you create all fields, which is used and checked in this test (as far as I see, checkMerges, retries and hasBeenMerged has no initialization)

[Mowinski]

Minor: You can move it to bottom of this class, just only for readability

[warcholprzemo]

Done