new datasource `azurerm_elastic_san_volume_snapshot` #26439

teowa · 2024-06-24T07:46:14Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

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

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

% ARM_TEST_ELASTIC_SAN_VOLUME_SNAPSHOT_RUN=1 make acctests SERVICE="elasticsan" TESTARGS="-parallel 5 -run TestAccElasticSANVolumeSnapshotData" TESTTIMEOUT='2h'                                                                                         ==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./internal/services/elasticsan -parallel 5 -run TestAccElasticSANVolumeSnapshotData -timeout 2h -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccElasticSANVolumeSnapshotDataSource_basic
--- PASS: TestAccElasticSANVolumeSnapshotDataSource_basic (336.60s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/elasticsan    336.633s

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

new datasource azurerm_elastic_san_volume_snapshot [GH-00000]

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Related Issue(s)

Fixes #0000

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

stephybun

Thanks @teowa. Could you take a look at the comments left in-line? Once that's done we can take another look through.

stephybun · 2024-06-24T13:10:05Z

internal/services/elasticsan/elastic_san_volume_snapshot_data_source.go

+
+type ElasticSANVolumeSnapshotDataSourceModel struct {
+	Name                string                                 `tfschema:"name"`
+	SourceVolumeSizeGiB int64                                  `tfschema:"source_volume_size_gib"`


In the other elastic san resources we're using something_in_gib, can we rename this to follow that pattern as well

Suggested change

SourceVolumeSizeGiB int64 `tfschema:"source_volume_size_gib"`

SourceVolumeSizeGiB int64 `tfschema:"source_volume_size_in_gib"`

stephybun · 2024-06-24T13:11:20Z

internal/services/elasticsan/elastic_san_volume_snapshot_data_source.go

+	}
+}
+
+func FlattenElasticSANVolumeSnapshotCreateSource(input snapshots.SnapshotCreationData) (*[]ElasticSANVolumeSnapshotCreateSource, error) {


Does this need to be a public function?

Suggested change

func FlattenElasticSANVolumeSnapshotCreateSource(input snapshots.SnapshotCreationData) (*[]ElasticSANVolumeSnapshotCreateSource, error) {

func flattenElasticSANVolumeSnapshotCreateSource(input snapshots.SnapshotCreationData) (*[]ElasticSANVolumeSnapshotCreateSource, error) {

the block is flattened, so this function is removed.

stephybun · 2024-06-24T13:13:33Z

internal/services/elasticsan/elastic_san_volume_snapshot_data_source.go

+	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


Since only ElasticSAN Volumes are supported for now can we simplify this for the time being

Suggested change

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

stephybun · 2024-06-24T13:21:15Z

internal/services/elasticsan/elastic_san_volume_snapshot_data_source.go

+		"creation_source": {
+			Type:     pluginsdk.TypeList,
+			Computed: true,
+			Elem: &pluginsdk.Resource{
+				Schema: map[string]*pluginsdk.Schema{
+					"source_id": {
+						Type:     pluginsdk.TypeString,
+						Computed: true,
+					},
+				},
+			},
+		},


Can we flatten this to

Suggested change

"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,

},

make sense, changed

stephybun · 2024-06-24T13:21:43Z

website/docs/d/elastic_san_volume_snapshot.html.markdown

+
+* `volume_group_id` - The Elastic SAN Volume Group ID within which the Elastic SAN Volume Snapshot exists.
+
+## Attributes Reference


The volume_name and source_volume_size_in_gib seem to be missing here

stephybun · 2024-06-24T13:24:35Z

internal/services/elasticsan/elastic_san_volume_snapshot_data_source_test.go

+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)
+	}


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:

terraform-provider-azurerm/internal/services/containers/kubernetes_node_pool_snapshot_data_source_test.go

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"),

),

},

stephybun · 2024-06-24T13:24:52Z

internal/services/elasticsan/elastic_san_volume_snapshot_data_source.go

+			ValidateFunc: validate.ElasticSanSnapshotName,
+		},
+
+		"volume_group_id": {


Can we add some validation here?

… step; validate volume_group_id; fix doc

teowa · 2024-06-26T04:25:58Z

Thanks for reviewing this, I have changed the code.

stephybun

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!

stephybun · 2024-07-08T08:12:38Z

internal/services/elasticsan/elastic_san_volume_snapshot_data_source.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)


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

Suggested change

return fmt.Errorf("parsing source ID as ElasticSAN Volume ID: %+v", err)

return err

stephybun · 2024-07-08T08:17:06Z

website/docs/d/elastic_san_volume_snapshot.html.markdown

+
+* `source_id` - The Resource ID from which the Snapshot is created.
+
+* `source_volume_size_gib` - Size of Source Volume.


Suggested change

* `source_volume_size_gib` - Size of Source Volume.

* `source_volume_size_in_gib` - The size of Source Volume.

stephybun · 2024-07-08T08:17:16Z

website/docs/d/elastic_san_volume_snapshot.html.markdown

+
+* `source_volume_size_gib` - Size of Source Volume.
+
+* `volume_name` - Source Volume Name of the Snapshot.


Suggested change

* `volume_name` - Source Volume Name of the Snapshot.

* `volume_name` - The source Volume Name of the Snapshot.

stephybun · 2024-07-08T08:19:30Z

internal/services/elasticsan/elastic_san_volume_snapshot_data_source_test.go

+variable "primary_location" {
+  default = %q
+}
+variable "random_integer" {
+  default = %d
+}
+variable "random_string" {
+  default = %q
+}


This looks like a remnant from a resource/data source generated with Pandora, please remove these

Suggested change

variable "primary_location" {

default = %q

}

variable "random_integer" {

default = %d

}

variable "random_string" {

default = %q

}

stephybun · 2024-07-08T08:20:48Z

internal/services/elasticsan/elastic_san_volume_snapshot_data_source_test.go

+  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


Suggested change

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

stephybun · 2024-07-08T08:21:57Z

internal/services/elasticsan/elastic_san_volume_snapshot_data_source_test.go

+variable "primary_location" {
+  default = %q
+}
+variable "random_integer" {
+  default = %d
+}
+variable "random_string" {
+  default = %q
+}


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.

…test

teowa · 2024-07-09T08:52:25Z

Hi @stephybun , thanks for reviewing this. I have changed the code, please kindly take another look.

TF_ACC=1 go test -v ./internal/services/elasticsan -parallel 5 -run TestAccElasticSANVolumeSnapshotData -timeout 2h -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccElasticSANVolumeSnapshotDataSource_basic
--- PASS: TestAccElasticSANVolumeSnapshotDataSource_basic (394.37s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/elasticsan    394.399s

stephybun

Thanks @teowa LGTM 👍

new datasource elastic_san_volume_snapshot

5bfb5b3

github-actions bot added documentation service/elasticsan size/XL labels Jun 24, 2024

doc: add empty line

0c23353

github-actions bot added the waiting-response label Jun 24, 2024

terrafmt data_source_test.go

699fc9b

github-actions bot removed the waiting-response label Jun 24, 2024

stephybun requested changes Jun 24, 2024

View reviewed changes

github-actions bot added the waiting-response label Jun 24, 2024

flatten sourceId; rename _tib to _in_tib; create snap in acctest test…

6b915e2

… step; validate volume_group_id; fix doc

github-actions bot removed the waiting-response label Jun 26, 2024

fix words

aaf61b0

stephybun requested changes Jul 8, 2024

View reviewed changes

github-actions bot added the waiting-response label Jul 8, 2024

fix doc; fix parse volumeID error message; remove variable use in acc…

eaa5c1c

…test

github-actions bot removed the waiting-response label Jul 9, 2024

stephybun approved these changes Jul 9, 2024

View reviewed changes

stephybun added the new-data-source label Jul 9, 2024

stephybun merged commit b298acc into hashicorp:main Jul 9, 2024
40 checks passed

github-actions bot added this to the v3.112.0 milestone Jul 9, 2024

stephybun added a commit that referenced this pull request Jul 9, 2024

Update for #26439

742999c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new datasource `azurerm_elastic_san_volume_snapshot` #26439

new datasource `azurerm_elastic_san_volume_snapshot` #26439

teowa commented Jun 24, 2024

stephybun left a comment

stephybun Jun 24, 2024

teowa Jun 26, 2024

stephybun Jun 24, 2024

teowa Jun 26, 2024

stephybun Jun 24, 2024

teowa Jun 26, 2024

stephybun Jun 24, 2024

teowa Jun 26, 2024

stephybun Jun 24, 2024 •

edited

Loading

teowa Jun 26, 2024

stephybun Jun 24, 2024

teowa Jun 26, 2024

stephybun Jun 24, 2024

teowa Jun 26, 2024

teowa commented Jun 26, 2024

stephybun left a comment

stephybun Jul 8, 2024

teowa Jul 9, 2024

stephybun Jul 8, 2024

teowa Jul 9, 2024

stephybun Jul 8, 2024

teowa Jul 9, 2024

stephybun Jul 8, 2024

teowa Jul 9, 2024

stephybun Jul 8, 2024

teowa Jul 9, 2024

stephybun Jul 8, 2024

teowa Jul 9, 2024

teowa commented Jul 9, 2024

stephybun left a comment

	SourceVolumeSizeGiB int64 `tfschema:"source_volume_size_gib"`
	SourceVolumeSizeGiB int64 `tfschema:"source_volume_size_in_gib"`

	func FlattenElasticSANVolumeSnapshotCreateSource(input snapshots.SnapshotCreationData) (*[]ElasticSANVolumeSnapshotCreateSource, error) {
	func flattenElasticSANVolumeSnapshotCreateSource(input snapshots.SnapshotCreationData) (*[]ElasticSANVolumeSnapshotCreateSource, error) {


		* `volume_group_id` - The Elastic SAN Volume Group ID within which the Elastic SAN Volume Snapshot exists.

		## Attributes Reference

	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"),
	),
	},

	return fmt.Errorf("parsing source ID as ElasticSAN Volume ID: %+v", err)
	return err


		* `source_id` - The Resource ID from which the Snapshot is created.

		* `source_volume_size_gib` - Size of Source Volume.

	* `source_volume_size_gib` - Size of Source Volume.
	* `source_volume_size_in_gib` - The size of Source Volume.


		* `source_volume_size_gib` - Size of Source Volume.

		* `volume_name` - Source Volume Name of the Snapshot.

	* `volume_name` - Source Volume Name of the Snapshot.
	* `volume_name` - The source Volume Name of the Snapshot.

new datasource azurerm_elastic_san_volume_snapshot #26439

new datasource azurerm_elastic_san_volume_snapshot #26439

Conversation

teowa commented Jun 24, 2024

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

stephybun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephybun Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teowa commented Jun 26, 2024

stephybun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teowa commented Jul 9, 2024

stephybun left a comment

Choose a reason for hiding this comment

new datasource `azurerm_elastic_san_volume_snapshot` #26439

new datasource `azurerm_elastic_san_volume_snapshot` #26439

stephybun Jun 24, 2024 •

edited

Loading