-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
new datasource azurerm_elastic_san_volume_snapshot
#26439
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.
Thanks @teowa. Could you take a look at the comments left in-line? Once that's done we can take another look through.
|
||
type ElasticSANVolumeSnapshotDataSourceModel struct { | ||
Name string `tfschema:"name"` | ||
SourceVolumeSizeGiB int64 `tfschema:"source_volume_size_gib"` |
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.
In the other elastic san resources we're using something_in_gib
, can we rename this to follow that pattern as well
SourceVolumeSizeGiB int64 `tfschema:"source_volume_size_gib"` | |
SourceVolumeSizeGiB int64 `tfschema:"source_volume_size_in_gib"` |
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.
changed
} | ||
} | ||
|
||
func FlattenElasticSANVolumeSnapshotCreateSource(input snapshots.SnapshotCreationData) (*[]ElasticSANVolumeSnapshotCreateSource, error) { |
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.
Does this need to be a public function?
func FlattenElasticSANVolumeSnapshotCreateSource(input snapshots.SnapshotCreationData) (*[]ElasticSANVolumeSnapshotCreateSource, error) { | |
func flattenElasticSANVolumeSnapshotCreateSource(input snapshots.SnapshotCreationData) (*[]ElasticSANVolumeSnapshotCreateSource, error) { |
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 block is flattened, so this function is removed.
var sourceId string | ||
parsedSourceId, err := volumes.ParseVolumeIDInsensitively(input.SourceId) | ||
if err != nil { | ||
return nil, fmt.Errorf("parsing source ID as ElasticSAN Volume ID: %+v", err) | ||
} | ||
sourceId = parsedSourceId.ID() | ||
|
||
return &[]ElasticSANVolumeSnapshotCreateSource{ | ||
{ | ||
SourceId: sourceId, | ||
}, | ||
}, nil |
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.
Since only ElasticSAN Volumes are supported for now can we simplify this for the time being
var sourceId string | |
parsedSourceId, err := volumes.ParseVolumeIDInsensitively(input.SourceId) | |
if err != nil { | |
return nil, fmt.Errorf("parsing source ID as ElasticSAN Volume ID: %+v", err) | |
} | |
sourceId = parsedSourceId.ID() | |
return &[]ElasticSANVolumeSnapshotCreateSource{ | |
{ | |
SourceId: sourceId, | |
}, | |
}, nil | |
volumeId, err := volumes.ParseVolumeIDInsensitively(input.SourceId) | |
if err != nil { | |
return nil, err | |
} | |
return &[]ElasticSANVolumeSnapshotCreateSource{ | |
{ | |
SourceId: volumeId.ID(), | |
}, | |
}, nil |
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.
changed
"creation_source": { | ||
Type: pluginsdk.TypeList, | ||
Computed: true, | ||
Elem: &pluginsdk.Resource{ | ||
Schema: map[string]*pluginsdk.Schema{ | ||
"source_id": { | ||
Type: pluginsdk.TypeString, | ||
Computed: true, | ||
}, | ||
}, | ||
}, | ||
}, |
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.
Can we flatten this to
"creation_source": { | |
Type: pluginsdk.TypeList, | |
Computed: true, | |
Elem: &pluginsdk.Resource{ | |
Schema: map[string]*pluginsdk.Schema{ | |
"source_id": { | |
Type: pluginsdk.TypeString, | |
Computed: true, | |
}, | |
}, | |
}, | |
}, | |
"source_id": { | |
Type: pluginsdk.TypeString, | |
Computed: true, | |
}, |
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.
make sense, changed
|
||
* `volume_group_id` - The Elastic SAN Volume Group ID within which the Elastic SAN Volume Snapshot exists. | ||
|
||
## Attributes Reference |
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 volume_name
and source_volume_size_in_gib
seem to be missing here
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.
added
func TestAccElasticSANVolumeSnapshotDataSource_basic(t *testing.T) { | ||
data := acceptance.BuildTestData(t, "data.azurerm_elastic_san_volume_snapshot", "test") | ||
d := ElasticSANVolumeSnapshotDataSource{} | ||
|
||
const SnapshotTestRunEnv = "ARM_TEST_ELASTIC_SAN_VOLUME_SNAPSHOT_RUN" | ||
|
||
if os.Getenv(SnapshotTestRunEnv) == "" { | ||
t.Skipf("skip the test as one or more of below environment variables are not specified: %q", SnapshotTestRunEnv) | ||
} |
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.
We should add the creation of the Snapshot as a test step in here instead of using the local-exec
provisioner to run az cli
. That way we don't need to set an environment variable to run this test. Here's an example of where we provision resources "outside" of Terraform in tests:
Lines 23 to 58 in d0f0e80
func TestAccDataSourceKubernetesNodePoolSnapshot_basic(t *testing.T) { | |
data := acceptance.BuildTestData(t, "data.azurerm_kubernetes_node_pool_snapshot", "test") | |
r := KubernetesNodePoolSnapshotDataSource{} | |
data.DataSourceTest(t, []acceptance.TestStep{ | |
{ | |
Config: r.snapshotSource(data), | |
Check: acceptance.ComposeTestCheckFunc( | |
data.CheckWithClientForResource(func(ctx context.Context, clients *clients.Client, state *terraform.InstanceState) error { | |
if _, ok := ctx.Deadline(); !ok { | |
var cancel context.CancelFunc | |
ctx, cancel = context.WithTimeout(ctx, 30*time.Minute) | |
defer cancel() | |
} | |
client := clients.Containers.SnapshotClient | |
poolId, err := agentpools.ParseAgentPoolID(state.ID) | |
if err != nil { | |
return err | |
} | |
id := snapshots.NewSnapshotID(poolId.SubscriptionId, poolId.ResourceGroupName, data.RandomString) | |
snapshot := snapshots.Snapshot{ | |
Location: data.Locations.Primary, | |
Properties: &snapshots.SnapshotProperties{ | |
CreationData: &snapshots.CreationData{ | |
SourceResourceId: utils.String(poolId.ID()), | |
}, | |
}, | |
} | |
_, err = client.CreateOrUpdate(ctx, id, snapshot) | |
if err != nil { | |
return fmt.Errorf("creating %s: %+v", id, err) | |
} | |
return nil | |
}, "azurerm_kubernetes_cluster_node_pool.source"), | |
), | |
}, |
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.
changed
ValidateFunc: validate.ElasticSanSnapshotName, | ||
}, | ||
|
||
"volume_group_id": { |
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.
Can we add some validation here?
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.
added
… step; validate volume_group_id; fix doc
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.
Thanks @teowa. I made another pass through and there are still a few things that need to be fixed up. Once that's done though this should be good to go!
// only ElasticSAN Volumes are supported for now | ||
volumeId, err := volumes.ParseVolumeIDInsensitively(model.Properties.CreationData.SourceId) | ||
if err != nil { | ||
return fmt.Errorf("parsing source ID as ElasticSAN Volume ID: %+v", err) |
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 parser functions return a lot of information already, including the string that the function is attempting to parse as well as the ID type that it's trying to parse it to, so this is redundant information that we would be printing in the error message, please try and avoid wrapping the error messages returned by the resource ID functions with redundant information
return fmt.Errorf("parsing source ID as ElasticSAN Volume ID: %+v", err) | |
return err |
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.
fixed
|
||
* `source_id` - The Resource ID from which the Snapshot is created. | ||
|
||
* `source_volume_size_gib` - Size of Source Volume. |
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.
* `source_volume_size_gib` - Size of Source Volume. | |
* `source_volume_size_in_gib` - The size of Source Volume. |
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.
changed
|
||
* `source_volume_size_gib` - Size of Source Volume. | ||
|
||
* `volume_name` - Source Volume Name of the Snapshot. |
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.
* `volume_name` - Source Volume Name of the Snapshot. | |
* `volume_name` - The source Volume Name of the Snapshot. |
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.
changed
variable "primary_location" { | ||
default = %q | ||
} | ||
variable "random_integer" { | ||
default = %d | ||
} | ||
variable "random_string" { | ||
default = %q | ||
} |
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.
This looks like a remnant from a resource/data source generated with Pandora, please remove these
variable "primary_location" { | |
default = %q | |
} | |
variable "random_integer" { | |
default = %d | |
} | |
variable "random_string" { | |
default = %q | |
} |
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.
removed
name = "acctestrg-esvg-${var.random_integer}" | ||
location = var.primary_location | ||
} | ||
|
||
resource "azurerm_elastic_san" "test" { | ||
name = "acctestes-${var.random_string}" | ||
resource_group_name = azurerm_resource_group.test.name | ||
location = azurerm_resource_group.test.location | ||
base_size_in_tib = 1 | ||
sku { | ||
name = "Premium_LRS" | ||
} | ||
} | ||
|
||
resource "azurerm_elastic_san_volume_group" "test" { | ||
name = "acctestesvg-${var.random_string}" | ||
elastic_san_id = azurerm_elastic_san.test.id | ||
} | ||
|
||
resource "azurerm_elastic_san_volume" "test" { | ||
name = "acctestesv-${var.random_string}" | ||
volume_group_id = azurerm_elastic_san_volume_group.test.id | ||
size_in_gib = 1 |
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.
name = "acctestrg-esvg-${var.random_integer}" | |
location = var.primary_location | |
} | |
resource "azurerm_elastic_san" "test" { | |
name = "acctestes-${var.random_string}" | |
resource_group_name = azurerm_resource_group.test.name | |
location = azurerm_resource_group.test.location | |
base_size_in_tib = 1 | |
sku { | |
name = "Premium_LRS" | |
} | |
} | |
resource "azurerm_elastic_san_volume_group" "test" { | |
name = "acctestesvg-${var.random_string}" | |
elastic_san_id = azurerm_elastic_san.test.id | |
} | |
resource "azurerm_elastic_san_volume" "test" { | |
name = "acctestesv-${var.random_string}" | |
volume_group_id = azurerm_elastic_san_volume_group.test.id | |
size_in_gib = 1 | |
name = "acctestrg-esvg-%[2]d" | |
location = %[1]s | |
} | |
resource "azurerm_elastic_san" "test" { | |
name = "acctestes-%[3]s" | |
resource_group_name = azurerm_resource_group.test.name | |
location = azurerm_resource_group.test.location | |
base_size_in_tib = 1 | |
sku { | |
name = "Premium_LRS" | |
} | |
} | |
resource "azurerm_elastic_san_volume_group" "test" { | |
name = "acctestesvg-%[3]s" | |
elastic_san_id = azurerm_elastic_san.test.id | |
} | |
resource "azurerm_elastic_san_volume" "test" { | |
name = "acctestesv-%[3]s" | |
volume_group_id = azurerm_elastic_san_volume_group.test.id | |
size_in_gib = 1 |
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.
changed
variable "primary_location" { | ||
default = %q | ||
} | ||
variable "random_integer" { | ||
default = %d | ||
} | ||
variable "random_string" { | ||
default = %q | ||
} |
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.
Same for this test config, remove the variables and refer to them using the indexed verbs notation like we do for other hand written data sources/resources in the provider.
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.
removed
Hi @stephybun , thanks for reviewing this. I have changed the code, please kindly take another look.
|
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.
Thanks @teowa LGTM 👍
<Actions> <action id="f410411e63aff4bb73a81c2aec1d373cf8a903e63b30dee2006b0030d8a94cc8"> <h3>Bump Terraform `azurerm` provider version</h3> <details id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24"> <summary>Update Terraform lock file</summary> <p>changes detected:
	"hashicorp/azurerm" updated from "3.111.0" to "3.112.0" in file ".terraform.lock.hcl"</p> <details> <summary>3.112.0</summary> <pre>Changelog retrieved from:
	https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.112.0
FEATURES:

* New Data Source: `azurerm_elastic_san_volume_snapshot` ([#26439](hashicorp/terraform-provider-azurerm#26439 New Resource: `azurerm_dev_center_dev_box_definition` ([#26307](hashicorp/terraform-provider-azurerm#26307 New Resource: `azurerm_dev_center_environment_type` ([#26291](hashicorp/terraform-provider-azurerm#26291 New Resource: `azurerm_virtual_machine_restore_point` ([#26526](hashicorp/terraform-provider-azurerm#26526 New Resource: `azurerm_virtual_machine_restore_point_collection` ([#26526](https://github.com/hashicorp/terraform-provider-azurerm/issues/26526))

ENHANCEMENTS:

* dependencies: updating to `v0.20240710.1114656` of `github.com/hashicorp/go-azure-sdk` ([#26588](hashicorp/terraform-provider-azurerm#26588 dependencies: updating to `v0.70.0` of `go-azure-helpers` ([#26601](hashicorp/terraform-provider-azurerm#26601 `containerservice`: updating the Fleet resources to use API Version `2024-04-01` ([#26588](hashicorp/terraform-provider-azurerm#26588 Data Source: `azurerm_network_service_tags` - extend validation for `service` to allow `AzureFrontDoor.Backend`, `AzureFrontDoor.Frontend`, and `AzureFrontDoor.FirstParty` ([#26429](hashicorp/terraform-provider-azurerm#26429 `azurerm_api_management_identity_provider_aad` - support for the `client_library` property ([#26093](hashicorp/terraform-provider-azurerm#26093 `azurerm_api_management_identity_provider_aadb2c` - support for the `client_library` property ([#26093](hashicorp/terraform-provider-azurerm#26093 `azurerm_dev_test_virtual_network` - support for the `shared_public_ip_address` property ([#26299](hashicorp/terraform-provider-azurerm#26299 `azurerm_kubernetes_cluster` - support for the `certificate_authority` block under the `service_mesh_profile` block ([#26543](hashicorp/terraform-provider-azurerm#26543 `azurerm_linux_web_app` - support the value `8.3` for the `php_version` property ([#26194](hashicorp/terraform-provider-azurerm#26194 `azurerm_machine_learning_compute_cluster` - the `identity` property can now be updated ([#26404](hashicorp/terraform-provider-azurerm#26404 `azurerm_web_application_firewall_policy` - support for the `JSChallenge` value for `managed_rules.managed_rule_set.rule_group_override.rule_action` ([#26561](https://github.com/hashicorp/terraform-provider-azurerm/issues/26561))

BUG FIXES:

* Data Source: `azurerm_communication_service` - `primary_connection_string`, `primary_key`, `secondary_connection_string` and `secondary_key` are marked as Sensitive ([#26560](hashicorp/terraform-provider-azurerm#26560 `azurerm_app_configuration_feature` - fix issue when updating the resource without an existing `targeting_filter` ([#26506](hashicorp/terraform-provider-azurerm#26506 `azurerm_backup_policy_vm` - split create and update function to fix lifecycle - ignore ([#26591](hashicorp/terraform-provider-azurerm#26591 `azurerm_backup_protected_vm` - split create and update function to fix lifecycle - ignore ([#26583](hashicorp/terraform-provider-azurerm#26583 `azurerm_communication_service` - the `primary_connection_string`, `primary_key`, `secondary_connection_string`, and `secondary_key` properties are now sensitive ([#26560](hashicorp/terraform-provider-azurerm#26560 `azurerm_mysql_flexible_server_configuration` - add locks to prevent conflicts when deleting the resource ([#26289](hashicorp/terraform-provider-azurerm#26289 `azurerm_nginx_deployment` - changing the `frontend_public.ip_address`, `frontend_private.ip_address`, `frontend_private.allocation_method`, and `frontend_private.subnet_id` now creates a new resource ([#26298](hashicorp/terraform-provider-azurerm#26298 `azurerm_palo_alto_local_rulestack_rule` - correctl read the `protocol` property on read when the `protocol_ports` property is configured ([#26510](hashicorp/terraform-provider-azurerm#26510 `azurerm_servicebus_namespace` - parse the identity returned by the API insensitively before setting into state ([#26540](https://github.com/hashicorp/terraform-provider-azurerm/issues/26540))

DEPRECATIONS:

* `azurerm_servicebus_queue` - `enable_batched_operations`, `enable_express` and `enable_partitioning` are superseded by `batched_operations_enabled`, `express_enabled` and `partitioning_enabled` ([#26479](hashicorp/terraform-provider-azurerm#26479 `azurerm_servicebus_subscription` - `enable_batched_operations` has been superseded by `batched_operations_enabled` ([#26479](hashicorp/terraform-provider-azurerm#26479 `azurerm_servicebus_topic` - `enable_batched_operations`, `enable_express` and `enable_partitioning` are superseded by `batched_operations_enabled`, `express_enabled` and `partitioning_enabled` ([#26479](https://github.com/hashicorp/terraform-provider-azurerm/issues/26479))


</pre> </details> </details> <a href="https://infra.ci.jenkins.io/job/updatecli/job/azure/job/main/319/">Jenkins pipeline link</a> </action> </Actions> --- <table> <tr> <td width="77"> <img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli logo" width="50" height="50"> </td> <td> <p> Created automatically by <a href="https://www.updatecli.io/">Updatecli</a> </p> <details><summary>Options:</summary> <br /> <p>Most of Updatecli configuration is done via <a href="https://www.updatecli.io/docs/prologue/quick-start/">its manifest(s)</a>.</p> <ul> <li>If you close this pull request, Updatecli will automatically reopen it, the next time it runs.</li> <li>If you close this pull request and delete the base branch, Updatecli will automatically recreate it, erasing all previous commits made.</li> </ul> <p> Feel free to report any issues at <a href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br /> If you find this tool useful, do not hesitate to star <a href="https://github.com/updatecli/updatecli/stargazers">our GitHub repository</a> as a sign of appreciation, and/or to tell us directly on our <a href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>! </p> </details> </td> </tr> </table> Co-authored-by: Jenkins Infra Bot (updatecli) <[email protected]>
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Community Note
Description
As discussed in #25372 (comment), the Elastic SAN Volume Snapshot should be onboarded as a data source instead of a resource. The acctest relys on Azure CLI to create/delete the snapshot, and an envrionment variable
ARM_TEST_ELASTIC_SAN_VOLUME_SNAPSHOT_RUN
is used to control whether to run the test.PR Checklist
For example: “
resource_name_here
- description of change e.g. adding propertynew_property_name_here
”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_elastic_san_volume_snapshot
[GH-00000]This is a (please select all that apply):
Related Issue(s)
Fixes #0000
Note
If this PR changes meaningfully during the course of review please update the title and description as required.