feat: Add Resource for External Volumes #3106
Conversation
| return diag.FromErr(err) | ||
| } | ||
|
|
||
| // Storage locations can only be added to the tail of the list, but can be |
There was a problem hiding this comment.
Have gone to the trouble of adding/removing storage locations here to avoid forcing new on the volume, as other objects will depend on it (e.g. iceberg tables). Implementation would be simpler if we simply rebuilt the entire storage location list each update, but that obviously has the potential to produce many more Snowflake queries.
| @@ -0,0 +1,1854 @@ | |||
| package resources_test | |||
|
|
|||
| // TODO Add test that includes Iceberg table creation, as this impacts the describe output (updates ACTIVE) | |||
There was a problem hiding this comment.
Would be good to have a test here that creates an Iceberg table, as this impacts the output of the ACTIVE property in the External Volume describe output. I would probably need your help to write this as Snowflake verifies access to the data on creation, so random values can’t be used for the bucket etc. - https://docs.snowflake.com/en/user-guide/tables-iceberg-storage#verifying-storage-access.
| var ( | ||
| s3StorageLocationName = "s3Test" | ||
| s3StorageLocationName2 = "s3Test2" | ||
| s3StorageProvider = "S3" |
There was a problem hiding this comment.
S3GOV acceptance tests are missing as I don’t have an awsgov deployment (same situation as in sdk integration tests).
| return diag.FromErr(err) | ||
| } | ||
|
|
||
| if withExternalChangesMarking { |
There was a problem hiding this comment.
As discussed on Slack, I’m not exactly sure how this works. It seems to be used for all Boolean values so I’ve copied it over and the tests are passing, would be good if you could check it.
| Optional: true, | ||
| Description: "Specifies the case-sensitive Amazon Resource Name (ARN) of the AWS identity and access management (IAM) role that grants privileges on the S3 bucket containing your data files.", | ||
| }, | ||
| "storage_aws_external_id": { |
There was a problem hiding this comment.
I’ve followed the example of Storage Integration, which leaves this as a read only parameter. However, for both the Storage Integration and the External Volume I can see use cases in the future where users would want to set it. We should keep that in mind for the next iteration.
| } | ||
|
|
||
| // TODO(SNOW-999142): Use SDK implementation for External Volume once it's available | ||
| // TODO switch to returning *sdk.ExternalVolume | ||
| // need to update existing acceptance tests for this | ||
| func (c *ExternalVolumeClient) Create(t *testing.T) (sdk.AccountObjectIdentifier, func()) { |
There was a problem hiding this comment.
Haven’t run integration tests for other tests that use this Create method. I don't actually use it in my tests, but nice to fix it up a bit if possible. I've still left the return type the same and the values hard coded to avoid having to refactor too much on this PR.
|
Hey @sfc-gh-asawicki / @sfc-gh-jcieslak / @sfc-gh-jmichalak 🙂 I’ll be on holidays for the next two weeks so won’t be able to respond to the review until I’m back. If you have the time you are welcome to take the PR over yourself to make changes / get it merged, otherwise I’ll take a look once I’m back. I’ve checked Cheers, |
| return nil, fmt.Errorf("unable to extract storage location, encryption_kms_key_id provided for azure storage location") | ||
| } | ||
|
|
||
| // TODO |
There was a problem hiding this comment.
Not sure exactly what's causing this issue, but possibly I’m being too strict trying to implement these checks - I can see there’s no equivalent in storage_integration.go. I thought it might be useful to fail fast rather than let terraform produce non empty plans for no reason, for example when specifying an azure_tenant_id on an s3 storage location. But I don’t want to over complicate it if it’s not necessary - whatever your team thinks is best 👍 . If we do want to implement them, this issue should definitely be looked into before merging.
| } | ||
| } | ||
|
|
||
| temp_storage_location, err := CopyStorageLocationWithTempName(removedLocations[0]) |
There was a problem hiding this comment.
It shouldn’t happen often but if deleting the temp resources fails in one apply, adding them on the next apply could fail due to duplicate names. If we want to avoid this, we could first run a check to see if the temp storage location already exists and run a remove operation.
This PR adds the resource for External Volumes. This is part of a series of PRs aimed at adding provider support for Iceberg tables.
References