PagerDuty / pdpyras

Low-level PagerDuty REST/Events API client for Python
MIT License
130 stars 30 forks source link

Events API : Payload overrides everything if set #17

Closed lindycoder closed 5 years ago

lindycoder commented 5 years ago

Hello!

I've been tinkering with the EventsAPISession and notice that if you set a payload it will completely ignore the summary, source and severity instead of merging it with the payload as the docstring says.

Here's the override: https://github.com/PagerDuty/pdpyras/blob/master/pdpyras.py#L641

I wouldn't mind setting either kwargs OR payload, but right now to make a valid event with any of the fields not supported by the kwargs (component, group and class) if have to do it like this because summary and source are not optional:

session = pdpyras.EventsAPISession(routing_key)

dedup_key = session.trigger(
    summary='ignored',
    source='ignored',
    payload={
        'summary': 'actual summary',
        'source': 'actual source',
        'severity': 'critical',
        'component': 'a component',
        'group': 'a group',
        'class': 'a class',
    },
    custom_details={
        'this': 'works'
    },
)

So maybe:

Just a suggestion, Thank you

Deconstrained commented 5 years ago

Hi @lindycoder,

Thank you for pointing this out. While the current documented behavior of EventsAPISession.trigger (as of the latest version) actually does specify that the contents of the payload argument override the other arguments that go into the default payload, I realize the way that it does this (complete replacement) is problematic and definitely not as documented. It doesn't merge with the default payload but rather replaces it.

Instead of this:

event['payload'] = payload

it should be doing this:

event['payload'].update(payload)

According to the docstring:

payload (dict) – Set the payload directly. Can be used in conjunction with other parameters that also set payload properties; these properties will be merged into the default payload, and any properties in this parameter will take precedence except with regard to custom_details

I do like your idea of switching to **kwargs, and might implement this.

Thanks again!

Deconstrained commented 5 years ago

New package (3.1.2) is published.

lindycoder commented 5 years ago

That was fast! Thank you very much!