Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where Grafana notification sends would fail with ValueError: invalid literal for int() with base 10 when dashboardId and panelId fields are left empty. The fix changes the conversion condition in GrafanaBackend.__init__ from checking is not None to checking != '', so that empty strings (as submitted by users via the UI) are treated as "not provided" rather than being passed to int().
Changes:
- Fix
int()conversion guard inGrafanaBackend.__init__to treat empty strings asNone. - Add a new
test_send_messages_with_emptyidstest case covering the empty-string scenario. - Update existing tests to pass
dashboardId=''andpanelId=''where IDs are not intended to be set, and update thepanelIdparametrize to use string values.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
awx/main/notifications/grafana_backend.py |
Changes the None-check to an empty-string check for dashboardId and panelId before calling int(). |
awx/main/tests/unit/notifications/test_grafana.py |
Adds test_send_messages_with_emptyids and updates existing tests to reflect the new string-based input convention. |
Comments suppressed due to low confidence (1)
awx/main/tests/unit/notifications/test_grafana.py:94
- The
test_send_messages_with_dashboardidparametrize still uses integer values[42, 0], while the correspondingtest_send_messages_with_panelidwas updated to use string values['42', '0']. Since the fix is intended to handle the case where these IDs come from user input as strings, thedashboardIdtest should also use string values like['42', '0']for consistency and to properly exercise theint(dashboardId)conversion in the backend.
Additionally, the assertion at line 94 compares against the raw dashboardId integer directly rather than int(dashboardId), which would hide a potential type mismatch if the parametrize were updated to strings.
@pytest.mark.parametrize("dashboardId", [42, 0])
def test_send_messages_with_dashboardid(dashboardId):
with mock.patch('awx.main.notifications.grafana_backend.requests') as requests_mock:
requests_mock.post.return_value.status_code = 200
m = {}
m['started'] = dt.datetime.utcfromtimestamp(60).isoformat()
m['finished'] = dt.datetime.utcfromtimestamp(120).isoformat()
m['subject'] = "test subject"
backend = grafana_backend.GrafanaBackend("testapikey", dashboardId=dashboardId, panelId='')
message = EmailMessage(
m['subject'],
{"started": m['started'], "finished": m['finished']},
[],
[
'https://example.com',
],
)
sent_messages = backend.send_messages(
[
message,
]
)
requests_mock.post.assert_called_once_with(
'https://example.com/api/annotations',
headers={'Content-Type': 'application/json', 'Authorization': 'Bearer testapikey'},
json={'text': 'test subject', 'isRegion': True, 'timeEnd': 120000, 'time': 60000, 'dashboardId': dashboardId},
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When Dashboard and Panel ID are left empty notifications would fail to send with ValueError: invalid literal for int() with base 10. This fix allows the optional fields to be empty.