-
Notifications
You must be signed in to change notification settings - Fork 79
Python SDK for ACLP Alerts #589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with respect to ACLP Alerting API specifications, logic, test cases and integration testing.
Requesting API / Cloud Firewall team to review https://github.com/linode/linode_api4-python/pull/589/files#diff-70a344c4146b66b96b63999ccef7d16b311cceba86cfc2ac987a5edf3cffcdc3R82-R87
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds comprehensive Monitor alerting SDK support to the Python API client, including CRUD operations for alert definitions, and improves test infrastructure to support both unit and integration testing scenarios.
- Adds full SDK support for Monitor alert definitions (create/get/update/delete operations)
- Fixes Enum serialization in JSONObject to prevent json.dumps errors
- Enhances test infrastructure with new fixtures and integration test resilience
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/groups/monitor_api_test.py | Adds comprehensive unit tests for alert definition CRUD operations |
| test/unit/base.py | Adds missing mock helper methods (GET, PUT, DELETE) for MonitorClientBaseCase |
| test/integration/models/monitor/test_monitor.py | Adds E2E integration test for alert definition lifecycle |
| test/integration/conftest.py | Makes firewall fixture skippable for local development |
| test/fixtures/*.json | Adds JSON fixtures for alert definition mocking |
| linode_api4/objects/serializable.py | Fixes Enum serialization to prevent JSON encoding errors |
| linode_api4/objects/monitor.py | Adds comprehensive data classes for alert definitions and channels |
| linode_api4/linode_client.py | Exposes MonitorGroup on MonitorClient for alert operations |
| linode_api4/groups/monitor.py | Implements alert definition CRUD methods |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
FIxed issues with CI and comments from Copilot AI |
|
To fix the lint issue, you can run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution! You probably need to make changes around the implementation of AlertDefinition
Co-authored-by: Ye Chen <[email protected]>
|
@yec-akamai all changes suggested are incorporated, commented whereever applicable, unit and integration test updated and tested. Thanks for the detailed review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests passed locally, excellent work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 14 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Addressed all comments from copilot and corrected unit and integration tests accordingly and also unit and integration tests are passing. |
|
All further comments from copilot are fixed, marked as not possible wherever not applicable. after that latest change unit and integration testing done and working as expected. |
|
@zliang-akamai @jriddle-linode @vshanthe request please review this and provide feedback and approval. |
|
@zliang-akamai @jriddle-linode @vshanthe request please review this and provide feedback and approval. any updates on 2nd review and approval? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@zliang-akamai the copilot comments are addressed , unit and integration test done and commited to this. |
| "id": Property(identifier=True), | ||
| "label": Property(), | ||
| "type": Property("channel_type"), | ||
| "type": Property(AlertType), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the first parameter of the constructor of Property class is mutable which is a boolean, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think its mutable = false by default in property constructor, and all the variables in this class are mutable = false only as its meant for read only and other CRUD opertation not supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why is AlertType passed in as the mutable argument?...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not clear. what is expected here , please suggest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As i read the spec for property mutable = false by default, this is only read API currently and no other CRUD operations are permitted in this.
hence mutable is false. not setting.
📝 Description
Add Monitor alerting SDK + fix/harden monitor unit & integration tests
What does this PR do and why is this change necessary?
This change is necessary because:
SDK additions (high level)
Note:
✔️ How to Test
Clone the repository
Prepare environment (zsh / macOS)
✔️create & activate venv (recommended)
python3 -m venv .venv
source .venv/bin/activate
install deps
python -m pip install --upgrade pip
Install runtime dependencies:
pip3 install requests polling deprecated
Install dev/test extras
pip3 install -e '.[dev,test]'
test deps
pip3 install pytest mock httpretty pytest-rerunfailures
✔️How do I run the relevant unit tests?
cd <PROJECT_DIRECTORY>/linode_api4-python
Unit tests: python -m pytest test/unit -q
377 passed, 18 warnings in 0.79s (as on 8Oct2025) new test cases may be added later
Unit test for all monitor: python -m pytest test/unit/groups/monitor_api_test.py -q -s
3 passed, 1 warning in 0.10s
Single unit: python -m pytest test/unit/groups/monitor_api_test.py::MonitorAlertDefinitionsTest -q -s
2 passed, 1 warning in 0.09s
What are the steps to reproduce the issue or verify the changes?
Its a new feature, so end to end integration should suffice
✔️How do I run the relevant integration tests?
Integration tests (E2E):
Note:
#if you have PAT token with write access only to Monitor and read for rest of the services for integration
export SKIP_E2E_FIREWALL=1 # optional: skip firewall autouse fixture
export LINODE_TOKEN="YOUR_REAL_TOKEN" # required for integration
export SKIP_E2E_FIREWALL=1 # optional: skip firewall autouse fixture
python -m pytest test/integration/models/monitor/test_monitor.py::test_integration_create_get_update_delete_alert_definition -q -s