Fix: add missing Return event ID from change event submission

[simonprydden]

changing from pdpyras to pagerduty library the return id is missing, the docstring of the function mentions the response ID

https://github.com/PagerDuty/pdpyras/blob/bb0790beb2831129c6ae018470a5e66a7e487611/pdpyras.py#L1475

    def send_change_event(self, **properties):
        """
        Send a change event to the v2 Change Events API.

        See: https://developer.pagerduty.com/docs/events-api-v2/send-change-events/

        :param **properties:
            Properties to set, i.e. ``payload`` and ``links``
        :returns:
            The response ID
        """
        event = deepcopy(properties)
        response = self.post('/v2/change/enqueue', json=event)
        response_body = try_decoding(successful_response(
            response,
            context="submitting change event",
        ))
        return response_body.get("id", None)

[simonprydden]

@Deconstrained Could you review this when you have time please?

[eladkal]

@Deconstrained any chance to review and have bug fix release for this?
During migration from pdpyras to python-pagerduty we detected this backward incompatible change apache/airflow#48436
Thus pager duty provider for Airflow can not use the new library till this is fixed.

[Deconstrained]

@simonprydden @eladkal Hello, and thank you for the pull request.

The API itself no longer returns a JSON with an ID, and so would return None instead (the response body schema is documented here). All Python functions without a return statement return None, which is the same as the default case for the call to dict.get, and so the code should have no functional difference.

Out of curiosity, is there is any hidden case I may be unaware of where the API response schema still contains an id property, and how is it used, or is this purely an issue that comes up in a CI/CD step i.e. some kind of static analysis?

[Deconstrained]

I'm forward-porting the fix to 2.0.0 so whichever comes first (fixing the access control issue that is blocking external pull requests or review/approval of #36) will determine if the fix comes as a patch release or new major release.

[simonprydden]

Hi @Deconstrained, this was picked up in the unit tests in the CI/CD step, but now I'm looking more at the API reference you linked, I think this change might not be correct.

Should the docstring for send_change_event return value be removed?

FYI the response for the live server & mock server are different, when I click the send api request button.

[Deconstrained]

Hi @Deconstrained, this was picked up in the unit tests in the CI/CD step, but now I'm looking more at the API reference you linked, I think this change might not be correct.

Should the docstring for send_change_event return value be removed?

Indeed, thank you for noticing this. I'll update the client documentation as well so it reflects reality.

Would it break the Airflow build if the function had an explicit return statement of None, or is there a test that expects it to return an ID? If the latter, the Airflow tests should eventually remove this expectation as the id property was removed.

As for the server side (mock vs live), I'm looking into how to resolve this.

[simonprydden]

Would it break the Airflow build if the function had an explicit return statement of None, or is there a test that expects it to return an ID? If the latter, the Airflow tests should eventually remove this expectation as the id property was removed.

It's a test that expects it to return an ID. I'll correct the tests to match the return value of none.

Lets close this PR, as I don't think it's needed? I was correcting the function to match the docstring, but we should be doing it the other way.

[simonprydden]

@Deconstrained Has send_change_event function ever returned an ID, or is this a long-standing mismatch between the documentation and the actual return value? git blame shows the function hasn’t changed in the last four years.

[Deconstrained]

@Deconstrained Has send_change_event function ever returned an ID, or is this a long-standing mismatch between the documentation and the actual return value? git blame shows the function hasn’t changed in the last four years.

The API originally may have had an id property in the response schema during the early access period. The reference to id was introduced in PagerDuty/pdpyras#56 (when it was first added).