ACLP Alerting get_channles() API changes and Added Enum for 'Status'#645
ACLP Alerting get_channles() API changes and Added Enum for 'Status'#645srbhaakamai wants to merge 4 commits intolinode:devfrom
Conversation
srbhaakamai
requested review from
ezilber-akamai and
yec-akamai
and removed request for
a team
This reverts commit d40b96c.
satkumar-akamai
left a comment
satkumar-akamai
left a comment
There was a problem hiding this comment.
Looks good to me as per api spec.
There was a problem hiding this comment.
Pull request overview
This PR updates the AlertChannel model to accommodate a recent API structure change in the ACLP Alerting service's get_channels() endpoint. The changes replace the previous alerts property structure with a new AlertsReference format and add a details property with channel-specific configuration. Additionally, the integration test is updated to use the new "provisioning" status value instead of "in progress".
Changes:
- Updated AlertChannel property definitions to use
AlertsReferenceandChannelDetailsinstead of the previousAlertsstructure - Added three new dataclasses (
EmailDetails,ChannelDetails,AlertsReference) to support the new API response format - Changed status check in integration test from "in progress" to "provisioning"
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| linode_api4/objects/monitor.py | Added new dataclasses for the updated AlertChannel API structure and modified AlertChannel properties to use AlertsReference and ChannelDetails |
| test/integration/models/monitor/test_monitor.py | Updated status value check from "in progress" to "provisioning" in alert readiness polling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class AlertsReference(JSONObject): | ||
| """ | ||
| Represents a reference to alerts associated with an alert channel. | ||
|
|
There was a problem hiding this comment.
There is a trailing whitespace after the opening docstring quotes. This should be removed to maintain consistent documentation formatting.
| alert = client.load(AlertDefinition, alert_id, service_type) | ||
| while ( | ||
| getattr(alert, "status", None) == "in progress" | ||
| getattr(alert, "status", None) == "provisioning" |
There was a problem hiding this comment.
The status value has been changed from "in progress" to "provisioning". This change aligns with the PR description indicating an API structure change. However, consider whether an enum for status values should be introduced to avoid hard-coded strings. This would make the code more maintainable and prevent typos.
yec-akamai
left a comment
yec-akamai
left a comment
There was a problem hiding this comment.
Looks good. will the Status enum be added too?
| "channel_type": Property(), | ||
| "alerts": Property(mutable=False, json_object=Alerts), | ||
| "details": Property(mutable=False, json_object=ChannelDetails), | ||
| "alerts": Property(mutable=False, json_object=AlertsReference), |
There was a problem hiding this comment.
nit: can you align the naming in python-sdk and linodego? It will be helpful for future maintenance. I remember in linodego you name the struct as AlertInfo but here is object class is AlertsReference. Can you just pick one naming and use it in both repo?
3 participants
📝 Description
This is to address ACLP Alerting get_channels() API structure change for Alertchannels which was released recently.
The changes will fix the issues which originated post release.
Also added new enumeration for Status in AlertDefinition.
✔️ How to Test
What are the steps to reproduce the issue or verify the changes?
How do I run the relevant unit/integration tests?
##INTEGRATION TEST
export LINODE_TOKEN=""
export SKIP_E2E_FIREWALL=1
(venv) srbha@blr-mpo40 linode_api4-python % python -m pytest test/integration/models/monitor/test_monitor.py::test_integration_create_get_update_delete_alert_definition -q -s
.
1 passed in 10.28s
##UNIT TEST
python3 -m pytest test/unit -q
441 passed, 34 warnings in 0.93s