feat: add HNS support for storage bucket #11852

gurusai-voleti · 2024-09-27T07:05:52Z

Adds support to enable hierarchical_namespace for storage buckets

Release Note Template for Downstream PRs (will be copied)

storage: added 'hierarchical_namespace' to 'google_storage_bucket' resource

google-cla · 2024-09-27T07:05:56Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

github-actions · 2024-09-27T07:06:33Z

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@rileykarson, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

kautikdk · 2024-09-27T07:29:07Z

mmv1/third_party/terraform/services/storage/resource_storage_bucket.go.tmpl

+			"hierarchical_namespace": {
+				Type:        schema.TypeList,
+				MaxItems:    1,
+				Optional:    true,


I think the whole block should be configured with ForceNew behavior. This block can not be changes or remove from the config once it is configured, Right?

Yes updated block with ForceNew

kautikdk · 2024-09-27T07:36:27Z

Apart from the field behavior comments, Please check bucket deletion. If the bucket is configured with force_destroy=true then while deleting bucket, it should delete all the folders and objects within the it. This may require changing logic in deletion operation.
Also please add documentation of the newly added fields and PR description as well.

kautikdk · 2024-09-27T07:31:25Z

mmv1/third_party/terraform/services/storage/resource_storage_bucket.go.tmpl

+					Schema: map[string]*schema.Schema{
+						"enabled": {
+							Type:        schema.TypeBool,
+							Default:     false,


No need to set Default: false, If it is not API default. I believe this field should marked with Required=true as I can see that empty "hierarchical_namespace" won't be returned in the API response.

removed Default

kautikdk · 2024-09-27T07:31:52Z

mmv1/third_party/terraform/services/storage/resource_storage_bucket.go.tmpl

+						"enabled": {
+							Type:        schema.TypeBool,
+							Default:     false,
+							Optional:    true,


Can remove this in favor of Required: true,

removed Optional and added required true

…o.tmpl

gurusai-voleti · 2024-09-30T10:29:48Z

Apart from the field behavior comments, Please check bucket deletion. If the bucket is configured with force_destroy=true then while deleting bucket, it should delete all the folders and objects within the it. This may require changing logic in deletion operation. Also please add documentation of the newly added fields and PR description as well.

->> ran manual test case locally as below

create HNS enabled bucket with force_destroy=true and upload files in bucket
check bucket is created with contents
when tried to delete bucket, terraform deleted the all the files in bucket and deleted the bucket

so no code changes needed for force_destroy for HNS bucket

update: added force destroy test case in acceptance tests

kautikdk · 2024-09-30T16:01:50Z

mmv1/third_party/terraform/services/storage/resource_storage_bucket.go.tmpl

+				Description: `The bucket's HNS support, which defines bucket can organize folders in logical file system structure`,
+				Elem : &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"enabled": {


Should we mark this field with ForceNew as well since it is not mutable?

currently this block contains only one resource I see no need of adding again here as the whole block is enforce with ForceNew true,

please suggest otherwise I will add ForceNew also at enabled field level @kautikdk

kautikdk · 2024-09-30T16:06:29Z

mmv1/third_party/terraform/services/storage/resource_storage_bucket.go.tmpl

@@ -880,6 +900,12 @@ func resourceStorageBucketUpdate(d *schema.ResourceData, meta interface{}) error
 		}
 	}

+	if d.HasChange("hierarchical_namespace") {
+		if v, ok := d.GetOk("hierarchical_namespace"); ok {
+			sb.HierarchicalNamespace = expandBucketHierachicalNamespace(v.([]interface{}))


I believe, There is no need for checking this block in update as the block is marked with ForceNew which will never allow having updates for the block.

removed this block

kautikdk · 2024-09-30T16:07:15Z

mmv1/third_party/terraform/services/storage/resource_storage_bucket_test.go.tmpl

+				Config: testAccStorageBucket_basic_hns(bucketName),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(
+						"google_storage_bucket.bucket", "hierarchical_namespace.0.enabled", "true"),


Since we are not adding update tests, It would be worth adding test for false value as well.

added false test case

kautikdk · 2024-09-30T16:08:24Z

mmv1/third_party/terraform/services/storage/resource_storage_bucket_test.go.tmpl

+`, bucketName)
+}
+
+func testAccStorageBucket_basic_hns_with_data(bucketName string, enabled bool, forceDestroy bool) string {


Not sure where are we adding data here?

in config we are creating the bucket and in check we are calling testAccCheckStorageBucketPutItem where a file is uploaded to bucket,

I debugged the test case with TF_LOG=DEBUG and it executed as below

bucket created

object uploaded to bucket

after test is successful, in infra cleaning step, as we enforced force_destroy true, objects got deleted and then bucket deleted
@kautikdk

hns bucket

d940e4f

github-actions bot requested a review from rileykarson September 27, 2024 07:06

modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Sep 27, 2024

gurusai-voleti marked this pull request as draft September 27, 2024 07:26

kautikdk reviewed Sep 27, 2024

View reviewed changes

gurusai-voleti added 3 commits September 27, 2024 09:49

mmv1/third_party/terraform/services/storage/resource_storage_bucket.g…

71da0ba

…o.tmpl

remove default

d0efd3c

Merge branch 'GoogleCloudPlatform:main' into main

f876dfc

gurusai-voleti added 4 commits September 30, 2024 12:27

add force destroy test case

469834f

update

4abfd7d

corrected params

63ebc1a

update documentation

f110095

kautikdk reviewed Sep 30, 2024

View reviewed changes

gurusai-voleti added 3 commits September 30, 2024 16:11

add example for hns

7f4d6da

remove checking change of hns in update block

93b765c

add hns bucket with false testcase

88c828d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add HNS support for storage bucket #11852

feat: add HNS support for storage bucket #11852

gurusai-voleti commented Sep 27, 2024 •

edited

Loading

google-cla bot commented Sep 27, 2024

github-actions bot commented Sep 27, 2024

kautikdk Sep 27, 2024

gurusai-voleti Sep 27, 2024

kautikdk commented Sep 27, 2024

kautikdk Sep 27, 2024

gurusai-voleti Sep 27, 2024

kautikdk Sep 27, 2024

gurusai-voleti Sep 27, 2024

gurusai-voleti commented Sep 30, 2024 •

edited

Loading

kautikdk Sep 30, 2024

gurusai-voleti Oct 1, 2024

kautikdk Sep 30, 2024

gurusai-voleti Sep 30, 2024

kautikdk Sep 30, 2024

gurusai-voleti Oct 1, 2024

kautikdk Sep 30, 2024

gurusai-voleti Sep 30, 2024 •

edited

Loading

feat: add HNS support for storage bucket #11852

Are you sure you want to change the base?

feat: add HNS support for storage bucket #11852

Conversation

gurusai-voleti commented Sep 27, 2024 • edited Loading

google-cla bot commented Sep 27, 2024

github-actions bot commented Sep 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kautikdk commented Sep 27, 2024

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

gurusai-voleti commented Sep 30, 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

Choose a reason for hiding this comment

gurusai-voleti Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

gurusai-voleti commented Sep 27, 2024 •

edited

Loading

gurusai-voleti commented Sep 30, 2024 •

edited

Loading

gurusai-voleti Sep 30, 2024 •

edited

Loading