reverted and changes Alert Channel changes as per latest API v4 changes#890
reverted and changes Alert Channel changes as per latest API v4 changes#890srbhaakamai wants to merge 3 commits intolinode:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Re-enables Monitor alert channel functionality and related integration tests, updating the Alert Channel model to match the latest API v4 shape.
Changes:
- Re-enabled integration tests that depend on listing alert channels and selecting a channel ID dynamically.
- Implemented
AlertChanneltypes (includingcontent/detailsblocks) and restoredListAlertChannels. - Added JSON unmarshalling for
created/updatedtimestamps usingparseabletime.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| test/integration/monitor_alert_definitions_test.go | Re-enables alert channel listing tests and uses live channel discovery for alert definition creation. |
| monitor_alert_channels.go | Restores alert channel API surface (types + ListAlertChannels) updated for API v4 response schema. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // endpoint := formatAPIPath("monitor/alert-channels") | ||
| // return getPaginatedResults[AlertChannel](ctx, c, endpoint, opts) | ||
| // } | ||
| type AlertNotificationType string |
There was a problem hiding this comment.
Several exported types are missing GoDoc-style comments (comments starting with the identifier name). If this repo uses golint/golangci-lint, this will typically fail linting; even if not, adding brief comments improves API readability for library consumers.
| EmailAlertNotification AlertNotificationType = "email" | ||
| ) | ||
|
|
||
| type AlertChannelType string |
There was a problem hiding this comment.
Several exported types are missing GoDoc-style comments (comments starting with the identifier name). If this repo uses golint/golangci-lint, this will typically fail linting; even if not, adding brief comments improves API readability for library consumers.
| } | ||
|
|
||
| // AlertsInfo represents alert information for a channel | ||
| type AlertsInfo struct { |
There was a problem hiding this comment.
Several exported types are missing GoDoc-style comments (comments starting with the identifier name). If this repo uses golint/golangci-lint, this will typically fail linting; even if not, adding brief comments improves API readability for library consumers.
| Type string `json:"type"` | ||
| AlertCount int `json:"alert_count"` | ||
| } | ||
|
|
There was a problem hiding this comment.
Several exported types are missing GoDoc-style comments (comments starting with the identifier name). If this repo uses golint/golangci-lint, this will typically fail linting; even if not, adding brief comments improves API readability for library consumers.
| // EmailChannelContent represents the content for email alert channels. |
| } | ||
|
|
||
| // ChannelDetails represents the details block for an AlertChannel | ||
| type ChannelDetails struct { |
There was a problem hiding this comment.
Several exported types are missing GoDoc-style comments (comments starting with the identifier name). If this repo uses golint/golangci-lint, this will typically fail linting; even if not, adding brief comments improves API readability for library consumers.
| } | ||
|
|
||
| // EmailChannelDetails represents email-specific details for a channel | ||
| type EmailChannelDetails struct { |
There was a problem hiding this comment.
Several exported types are missing GoDoc-style comments (comments starting with the identifier name). If this repo uses golint/golangci-lint, this will typically fail linting; even if not, adding brief comments improves API readability for library consumers.
| var channelID int | ||
| var fetchedChannelLabel string | ||
| var fetchedChannelID int |
There was a problem hiding this comment.
The test keeps three variables (channelID, fetchedChannelID, fetchedChannelLabel) but channelID and fetchedChannelID are always assigned to the same value. Consider storing the selected channel in a single local (e.g., selectedChannel) or asserting directly on channelID plus a single label value to reduce duplication and make the test intent clearer.
|
satkumar-akamai
left a comment
There was a problem hiding this comment.
Changes looks good as per API Spec.
yec-akamai
left a comment
There was a problem hiding this comment.
Overall looks good. Can you fix the comments please?
| @@ -208,33 +208,33 @@ func TestMonitorAlertDefinitions_List(t *testing.T) { | |||
| } | |||
|
|
|||
| // TODO: Disable this test until we can query alert channels correctly. | |||
There was a problem hiding this comment.
nit: can you clean up the TODO comment as well?
| // TODO: use a fixed channel id for now until the alert channel has been fixed. | ||
| //// Get a channel ID to use |
There was a problem hiding this comment.
nit: can you clean up the TODO comment as well?
| @@ -48,27 +48,27 @@ func TestMonitorAlertDefinition_smoke(t *testing.T) { | |||
| assert.NoError(t, err) | |||
|
|
|||
| // TODO: Use a fixed channel id for now until the alert channel has been fixed. | |||
There was a problem hiding this comment.
nit: can you clean up the TODO comment as well?
| c.BaseURL = "https://api.linode.com/v4/" | ||
| } | ||
|
|
||
| // SetUpWithAPIVersion initializes the Linode client with a specific API version |
There was a problem hiding this comment.
It's not necessary to have this function. Our integration test is default to use v4beta, and unit test does not check the api version though.
|
|
||
| // ChannelContent represents the content block for an AlertChannel, which varies by channel type. | ||
| type ChannelContent struct { | ||
| Email *EmailChannelContent `json:"email,omitempty"` |
There was a problem hiding this comment.
omitempty is not necessary if the field is read-only. Same for the rest fields.
yec-akamai
left a comment
There was a problem hiding this comment.
Can you also run the integration test and re-generate the fixtures please?
| AlertDefinitionStatusProvisioning AlertDefinitionStatus = "provisioning" | ||
| AlertDefinitionStatusEnabling AlertDefinitionStatus = "enabling" | ||
| AlertDefinitionStatusDisabling AlertDefinitionStatus = "disabling" |
There was a problem hiding this comment.
Just curious, is this already a change in the API response? I remember we didn't have these three status in the old response and they were represented by in_progress. If this is the case we will need to make changes to a wait_for function though.
📝 Description
What does this PR do and why is this change necessary?
✔️ How to Test
What are the steps to reproduce the issue or verify the changes?
How do I run the relevant unit/integration tests?
srbha@blr-mpo40 linodego % export LINODE_TOKEN="" && export LINODE_API_VERSION="v4beta" && go test -count=1 ./test/integration -run "TestMonitorAlertDefinition" -v
=== RUN TestMonitorAlertDefinition_smoke
--- PASS: TestMonitorAlertDefinition_smoke (66.19s)
=== RUN TestMonitorAlertDefinitions_List
--- PASS: TestMonitorAlertDefinitions_List (1.37s)
=== RUN TestMonitorAlertDefinition_CreateWithIdempotency
--- PASS: TestMonitorAlertDefinition_CreateWithIdempotency (3.88s)
PASS
ok github.com/linode/linodego/test/integration 72.096s
📷 Preview
If applicable, include a screenshot or code snippet of this change. Otherwise, please remove this section.