It would be nice to add a comment explaining why we ignore this _type.

We're no longer handling the case where data['item'] returns a falsy value for brew-build and koji_build subject types. Maybe that's ok?

Let's continue to use dedent here.

This is actually explained in the comment above (that was already there) - note: it is intentional that we do not handle old format etc...
Should I repeat it here or make it more explicit?

Maybe I didn't understand what do you mean.
Before we had: _type == 'koji_build' and 'item' in data (or brew-build, same case)
...we only used to check if 'item' is in data, not the value of it. Am I wrong?

I've rebased for the dedent change. Thanks for pointing that out.

Missing * at beginning of these paragraphs, if this these are part of the list "with the following differences:".

Also for consistency:

s/"subject_type"/``subject_type``/

This is actually explained in the comment above (that was already there) - note: it is intentional that we do not handle old format etc...
Should I repeat it here or make it more explicit?

It's probably fine to leave it off.

Maybe I didn't understand what do you mean.
Before we had: _type == 'koji_build' and 'item' in data (or brew-build, same case)
...we only used to check if 'item' is in data, not the value of it. Am I wrong?

The code used to say:

if (_type == 'koji_build' and 'item' in data or             
        _type == 'brew-build' and 'item' in data or         
        'original_spec_nvr' in data):                       
    if _type in ['koji_build', 'brew-build']:               
        nvr = _decode(data['item'])                         
    else:                                                   
        nvr = _decode(data['original_spec_nvr'])            
    # when the pipeline ignores a package, which happens    
    # *a lot*, we get a message with an 'original_spec_nvr' 
    # key with an empty value; let's not try and handle this
    if nvr:                                                 
        yield ('koji_build', nvr)                           

Note how it only emits a value if nvr is truthy. In the new version, if nvr is falsey and subject_type is one of brew-build or koji_build, the code will yield a result.

Actually those are not part of the "differences"... It's just new text. The differences were 3, now just one. I could maybe rephrase it.. bullet points don't make anymore sense now that it is a single bullet point

Maybe I didn't understand what do you mean.
Before we had: _type == 'koji_build' and 'item' in data (or brew-build, same case)
...we only used to check if 'item' is in data, not the value of it. Am I wrong?

The code used to say:
if (_type == 'koji_build' and 'item' in data or
_type == 'brew-build' and 'item' in data or
'original_spec_nvr' in data):
if _type in ['koji_build', 'brew-build']:
nvr = _decode(data['item'])
else:
nvr = _decode(data['original_spec_nvr'])
# when the pipeline ignores a package, which happens
# a lot, we get a message with an 'original_spec_nvr'
# key with an empty value; let's not try and handle this
if nvr:
yield ('koji_build', nvr)

Note how it only emits a value if nvr is truthy. In the new version, if nvr is falsey and subject_type is one of brew-build or koji_build, the code will yield a result.

Oh ok! I got what you mean.
I dropped it in the new version because the "if nvr:..." was there to avoid to yield with an empty nvr, but that should happen only when we have an "original_spec_nvr"... it shouldn't happen for other cases...

I've rebased with the suggestion about the doc from Lukas.
Should I change something else?

+1

:+1:

How about linking to https://docs.pagure.org/greenwave/policies.html#subject-types and updating that instead? That way this list is in a single location.

Good point. Thank you, Matt. I've filed this issue: https://pagure.io/greenwave/issue/363