Skip to content
This repository has been archived by the owner on Dec 5, 2023. It is now read-only.

Fixed no-namespace bug in Alert model #2

Merged
merged 2 commits into from
Apr 26, 2022

Conversation

abyrne55
Copy link
Contributor

There's a bug in the database event hook Alert._set_firing_details() that causes Alert database record creation to fail if a PagerDuty alert's firing_details field does not contain namespace info. The specific error is as follows:

ERROR:__main__:Failed to process alert Q0XYZTXYZIA6P
Traceback (most recent call last):
  File "/opt/app-root/src/updater.py", line 128, in update_alerts
    alert = Alert.from_pd_api_response(
  File "/opt/app-root/src/models.py", line 396, in from_pd_api_response
    alert.firing_details = res_dict["body"]["details"]["firing"]
  File "/opt/app-root/lib64/python3.9/site-packages/sqlalchemy/orm/attributes.py", line 459, in __set__
    self.impl.set(
  File "/opt/app-root/lib64/python3.9/site-packages/sqlalchemy/orm/attributes.py", line 1097, in set
    value = self.fire_replace_event(
  File "/opt/app-root/lib64/python3.9/site-packages/sqlalchemy/orm/attributes.py", line 1105, in fire_replace_event
    value = fn(
  File "/opt/app-root/lib64/python3.9/site-packages/sqlalchemy/orm/events.py", line 2266, in wrap
    fn(target, *arg)
  File "/opt/app-root/src/models.py", line 419, in _set_firing_details
    target.namespace = namespace_re.group(1)
AttributeError: 'NoneType' object has no attribute 'group'

This is due to an AttributeError raised when the code tries to fetch a non-existent regex result. With this PR, that exception will now be caught and logged as a warning. This PR also adds a sample alert that triggers this error to sample_incident_alerts.json and updates the unit tests for the Alert model such that they can now handle these multi-alert sample files.

This PR addresses bug OSD-10952.

test_models.py now catches the bug outlined in OSD-10592. Added a second
alert to sample_incident_alerts that has incomplete firing details, and
modified test_from_pd_api_response to test multiple alerts.
Also fixed test to only check for the namespace if one exists in the
firing details
@abyrne55 abyrne55 added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Apr 21, 2022
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 21, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 21, 2022

@abyrne55: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 26, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 26, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abyrne55, fahlmant

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 8732b5b into openshift:main Apr 26, 2022
@abyrne55 abyrne55 deleted the OSD-10952 branch April 26, 2022 13:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants