feat(block): add support for identity for snapshot and volume#3656
feat(block): add support for identity for snapshot and volume#3656remyleone wants to merge 1 commit intoscaleway:mainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3656 +/- ##
========================================
+ Coverage 2.34% 2.43% +0.09%
========================================
Files 453 469 +16
Lines 49798 54631 +4833
========================================
+ Hits 1167 1330 +163
- Misses 48540 53205 +4665
- Partials 91 96 +5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Pull request overview
This PR adds Terraform ResourceIdentity support for Scaleway Block Storage volumes and snapshots, aligning these resources with the provider’s identity-based import/state conventions.
Changes:
- Add
Identity: identity.DefaultZonal()to block volume and snapshot resources and populate identity viaidentity.SetZonalIdentity. - Refactor block volume/snapshot data sources to read via
waitForBlockVolume/waitForBlockSnapshotand reuse shared state-setting helpers. - Update
TestSDKProvider_ResourceIdentityNotEmptyto removescaleway_block_snapshot/scaleway_block_volumefrom the exception list.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| provider/sdkv2_test.go | Removes block snapshot/volume from the “no identity” exceptions list. |
| internal/services/block/volume.go | Adds zonal identity support and extracts setVolumeState helper. |
| internal/services/block/snapshot.go | Adds zonal identity support and extracts setSnapshotState helper. |
| internal/services/block/volume_data_source.go | Refactors data source read path to use waiter + shared state setter. |
| internal/services/block/snapshot_data_source.go | Refactors data source read path to use waiter + shared state setter. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Wait for the volume and use it to set the state | ||
| volume, err := waitForBlockVolume(ctx, api, zone, volumeID.(string), d.Timeout(schema.TimeoutRead)) | ||
| if err != nil { |
There was a problem hiding this comment.
waitForBlockVolume is called with volumeID.(string), but volume_id is stored in state as a zoned ID (e.g. fr-par-1/<uuid>). On subsequent reads (or when the user provides a localized ID), this will pass the localized form to the Block API, which expects the raw UUID, and it also ignores the zone embedded in the ID. Parse the zoned ID (e.g. via zonal.ParseID / locality.ExpandID) and use the extracted id (and zone if present) when calling waitForBlockVolume.
| d.SetId("") | ||
|
|
||
| return nil |
There was a problem hiding this comment.
For a data source, swallowing a 404 by clearing d.SetId("") and returning nil will typically lead to confusing downstream behavior (empty/unknown data) instead of a clear failure. The rest of the codebase’s data sources return a "... not found" diagnostic when the remote object doesn’t exist; consider returning a diag.Errorf here (including the zoned ID) rather than silently succeeding.
| d.SetId("") | |
| return nil | |
| return diag.Errorf("block volume %s not found", zoneID) |
| // Wait for the snapshot and use it to set the state | ||
| snapshot, err := waitForBlockSnapshot(ctx, api, zone, snapshotID.(string), d.Timeout(schema.TimeoutRead)) | ||
| if err != nil { |
There was a problem hiding this comment.
waitForBlockSnapshot is called with snapshotID.(string), but snapshot_id is stored in state as a zoned ID (e.g. fr-par-1/<uuid>). On subsequent reads (or when the user provides a localized ID), this will pass the localized form to the Block API, which expects the raw UUID, and it also ignores the zone embedded in the ID. Parse the zoned ID (e.g. via zonal.ParseID / locality.ExpandID) and use the extracted id (and zone if present) when calling waitForBlockSnapshot.
| d.SetId("") | ||
|
|
||
| return nil |
There was a problem hiding this comment.
For a data source, swallowing a 404 by clearing d.SetId("") and returning nil will typically lead to confusing downstream behavior (empty/unknown data) instead of a clear failure. The rest of the codebase’s data sources return a "... not found" diagnostic when the remote object doesn’t exist; consider returning a diag.Errorf here (including the zoned ID) rather than silently succeeding.
| d.SetId("") | |
| return nil | |
| return diag.Errorf("snapshot %s not found", zoneID) |
No description provided.