Don't panic on non-existant import #1579
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a panic that occurred when attempting to import a non-existent ML datafeed resource. The fix adds a nil check in the read operation to gracefully handle cases where the datafeed is not found, preventing the panic and allowing Terraform to properly report that the resource does not exist.
Key Changes:
- Added nil check in the datafeed read operation to handle non-existent resources
- Added acceptance test to verify the fix for importing non-existent datafeeds
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/elasticsearch/ml/datafeed/read.go | Added nil check after API call to prevent panic when datafeed doesn't exist |
| internal/elasticsearch/ml/datafeed/acc_test.go | Added acceptance test for importing non-existent datafeed |
| internal/elasticsearch/ml/datafeed/testdata/TestAccResourceDatafeed_ImportNonExistent/import_missing/datafeed.tf | Test configuration file for the non-existent import test case |
| resource "elasticstack_elasticsearch_ml_datafeed" "test" { | ||
| datafeed_id = var.datafeed_id | ||
| job_id = var.job_id |
There was a problem hiding this comment.
The test configuration references a non-existent job via var.job_id set to 'dummy-job'. Since the test is specifically for importing a non-existent datafeed, it should not depend on a non-existent job. Consider creating an actual ML job resource in the test configuration or document why the non-existent job reference is acceptable for this test scenario.
| ResourceName: "elasticstack_elasticsearch_ml_datafeed.test", | ||
| ImportState: true, | ||
| ImportStateId: "cluster-id/non-existent-datafeed-id", | ||
| ImportStateVerify: false, |
There was a problem hiding this comment.
The test disables import state verification with ImportStateVerify: false. For a test validating import behavior of non-existent resources, consider adding ExpectError to verify that Terraform properly reports the resource as not found, rather than silently succeeding with verification disabled.
|
|
||
| ### Changes | ||
|
|
||
| - Add `advanced_settings` to `elasticstack_fleet_agent_policy` to configure agent logging, CPU limits, and download settings ([#1545](https://github.com/elastic/terraform-provider-elasticstack/pull/1545)) |
Fixes #1570