Support Linode Integrations with ACLP Alerts#879
Support Linode Integrations with ACLP Alerts#879yec-akamai wants to merge 3 commits intoproj/aclp-linode-alertsfrom
Conversation
yec-akamai
requested review from
Copilot and
psnoch-akamai
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
This PR adds support for ACLP (Access Control List Policy) alerts to Linode instance operations, enabling system and user alert configurations during instance creation, updates, and cloning.
Changes:
- Extended
InstanceAlertstruct to includeSystemAlertsandUserAlertsfields - Added
InstanceACLPAlertsOptionstype for configuring alerts during instance creation and cloning - Updated instance operation options (
InstanceCreateOptions,InstanceUpdateOptions,InstanceCloneOptions) to support alert configuration - Added comprehensive test coverage for alert functionality across Get, Create, Update, and Clone operations
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| instances.go | Added ACLP alert fields to InstanceAlert struct and new InstanceACLPAlertsOptions type for alert configuration in create/clone operations |
| test/unit/instance_test.go | Added test assertions to verify SystemAlerts and UserAlerts are properly handled in Get, Create, Update, and Clone operations |
| test/unit/fixtures/instance_get.json | Added alerts field with system_alerts and user_alerts arrays to mock response data |
| test/unit/fixtures/instance_create.json | Added alerts field with system_alerts and user_alerts arrays to mock response data |
| test/unit/fixtures/instance_update.json | Added alerts field with system_alerts and user_alerts arrays to mock response data |
| test/unit/fixtures/instance_clone.json | Added alerts field with system_alerts and user_alerts arrays to mock response data |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Alerts *InstanceACLPAlertsOptions `json:"alerts,omitempty"` | ||
| } | ||
|
|
||
| // InstanceACLPAlertsOptions represents ACLP alerts options for instance creation and cloning. |
There was a problem hiding this comment.
The InstanceCreateOptions.Alerts field uses InstanceACLPAlertsOptions, but InstanceUpdateOptions.Alerts (line 226 in the test file) uses InstanceAlert. This inconsistency creates confusion about which type should be used for updates versus creation. Consider using the same type for both operations or clearly documenting why different types are needed.
| Alerts *InstanceACLPAlertsOptions `json:"alerts,omitempty"` | |
| } | |
| // InstanceACLPAlertsOptions represents ACLP alerts options for instance creation and cloning. | |
| // Note: This field intentionally uses InstanceACLPAlertsOptions, which differs | |
| // from the alerts type used in InstanceUpdateOptions (InstanceAlert). The API | |
| // for configuring alerts at creation/cloning time is distinct from the API | |
| // used when updating an existing Instance's alerts. | |
| Alerts *InstanceACLPAlertsOptions `json:"alerts,omitempty"` | |
| } | |
| // InstanceACLPAlertsOptions represents ACLP alerts options for instance creation | |
| // and cloning. This type is distinct from the alerts type used on | |
| // InstanceUpdateOptions (InstanceAlert) to reflect differences in the API for | |
| // initial (create/clone) configuration versus subsequent updates. |
There was a problem hiding this comment.
This is API inconsistency in creating and updating request.
There was a problem hiding this comment.
If InstanceACLPAlertsOptions is dedicated for updating, can we name it as InstanceACLPAlertsUpdateOptions?
| SystemAlerts *[]int `json:"system_alerts,omitempty"` | ||
| UserAlerts *[]int `json:"user_alerts,omitempty"` |
There was a problem hiding this comment.
I think a slice type ([]int) itself is already nil-able, right? Is there another reason to use a pointer to a slice type (*[]int) here?
3 participants
📝 Description
Add ACLP alerts support to linode instance operations.
✔️ How to Test