-
Notifications
You must be signed in to change notification settings - Fork 12
[DTC] Resource and Datasource Implementation for DTC Monitor ICMP #274
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: feature
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.
Pull request overview
This pull request implements a new resource and data source for DTC Monitor ICMP in the Terraform NIOS provider. The implementation follows the established patterns used for other DTC resources (LBDN, Server, Pool) in the codebase, providing full CRUD operations and comprehensive test coverage.
Key changes:
- Adds
nios_dtc_monitor_icmpresource with support for ICMP health monitoring configuration - Adds
nios_dtc_monitor_icmpdata source with filtering capabilities - Includes comprehensive test coverage for all resource attributes and data source filters
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/service/dtc/model_dtc_monitor_icmp.go | Defines the data model, schema attributes, and Expand/Flatten methods for DTC Monitor ICMP |
| internal/service/dtc/dtc_monitor_icmp_resource.go | Implements CRUD operations for the DTC Monitor ICMP resource |
| internal/service/dtc/dtc_monitor_icmp_resource_test.go | Provides comprehensive acceptance tests for all resource attributes |
| internal/service/dtc/dtc_monitor_icmp_data_source.go | Implements the data source with filtering and paging support |
| internal/service/dtc/dtc_monitor_icmp_data_source_test.go | Tests data source filtering capabilities |
| internal/provider/provider.go | Registers the new resource and data source with the provider |
| examples/resources/nios_dtc_monitor_icmp/resource.tf | Provides usage examples for the resource |
| examples/data-sources/nios_dtc_monitor_icmp/data-source.tf | Provides usage examples for the data source |
| docs/resources/dtc_monitor_icmp.md | Generated documentation for the resource |
| docs/data-sources/dtc_monitor_icmp.md | Generated documentation for the data source |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…open#265) Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.43.0 to 0.45.0. - [Commits](golang/crypto@v0.43.0...v0.45.0) --- updated-dependencies: - dependency-name: golang.org/x/crypto dependency-version: 0.45.0 dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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 10 out of 10 changed files in this pull request and generated 15 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| if associateInternalId != nil { | ||
| data.ExtAttrs, diags = AddInternalIDToExtAttrs(ctx, data.ExtAttrs, diags) | ||
| if diags.HasError() { |
Copilot
AI
Dec 1, 2025
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.
When diags.HasError() is true on line 262, the diagnostics are checked but never appended to resp.Diagnostics before returning. This means the error details won't be communicated to the user. Add resp.Diagnostics.Append(diags...) before the return statement on line 263.
| if diags.HasError() { | |
| if diags.HasError() { | |
| resp.Diagnostics.Append(diags...) |
| customvalidator.ValidateTrimmedString(), | ||
| stringvalidator.LengthBetween(0, 256), |
Copilot
AI
Dec 1, 2025
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.
[nitpick] The order of validators should be consistent with similar DTC monitor resources. In model_dtc_monitor_snmp.go (lines 76-79), the stringvalidator.LengthBetween validator is placed before customvalidator.ValidateTrimmedString(). Consider reordering these validators to maintain consistency across the codebase.
| customvalidator.ValidateTrimmedString(), | |
| stringvalidator.LengthBetween(0, 256), | |
| stringvalidator.LengthBetween(0, 256), | |
| customvalidator.ValidateTrimmedString(), |
|
|
||
| // Add internal ID exists in the Extensible Attributes if not already present | ||
| data.ExtAttrs, diags = AddInternalIDToExtAttrs(ctx, data.ExtAttrs, diags) | ||
| if diags.HasError() { |
Copilot
AI
Dec 1, 2025
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.
When diags.HasError() is true on line 77, the diagnostics are checked but never appended to resp.Diagnostics before returning. This means the error details won't be communicated to the user. Add resp.Diagnostics.Append(diags...) before the return statement on line 78.
| if diags.HasError() { | |
| if diags.HasError() { | |
| resp.Diagnostics.Append(diags...) |
| "extattrs_all": schema.MapAttribute{ | ||
| Computed: true, | ||
| ElementType: types.StringType, | ||
| MarkdownDescription: "Extensible attributes associated with the object , including default attributes.", |
Copilot
AI
Dec 1, 2025
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.
There is an extra space before the comma in "object , including". It should be "object, including".
| MarkdownDescription: "Extensible attributes associated with the object , including default attributes.", | |
| MarkdownDescription: "Extensible attributes associated with the object, including default attributes.", |
| res := apiRes.CreateDtcMonitorIcmpResponseAsObject.GetResult() | ||
| res.ExtAttrs, data.ExtAttrsAll, diags = RemoveInheritedExtAttrs(ctx, data.ExtAttrs, *res.ExtAttrs) | ||
| if diags.HasError() { | ||
| resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Error while create DtcMonitorIcmp due inherited Extensible attributes, got error: %s", err)) |
Copilot
AI
Dec 1, 2025
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.
The error message references err which is out of scope and will display an incorrect error. The err variable is from line 88 which checks the API call, but this error occurs when RemoveInheritedExtAttrs fails (line 94). The error message should reference diags instead: fmt.Sprintf("Error while create DtcMonitorIcmp due inherited Extensible attributes, got error: %s", diags)
| resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Error while create DtcMonitorIcmp due inherited Extensible attributes, got error: %s", err)) | |
| resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Error while create DtcMonitorIcmp due inherited Extensible attributes, got error: %s", diags)) |
|
|
||
| res.ExtAttrs, data.ExtAttrsAll, diags = RemoveInheritedExtAttrs(ctx, data.ExtAttrs, *res.ExtAttrs) | ||
| if diags.HasError() { | ||
| resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Error while reading DtcMonitorIcmp due inherited Extensible attributes, got error: %s", diags)) |
Copilot
AI
Dec 1, 2025
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.
The error message contains a grammatical error: "due inherited" should be "due to inherited". Corrected message: "Error while reading DtcMonitorIcmp due to inherited Extensible attributes"
| resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Error while reading DtcMonitorIcmp due inherited Extensible attributes, got error: %s", diags)) | |
| resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Error while reading DtcMonitorIcmp due to inherited Extensible attributes, got error: %s", diags)) |
No description provided.