Add provider/google/google_storage_bucket lifecycle interface #6

pdecat · 2017-06-12T12:51:21Z

This PR adds support for the lifecycle of google_storage_bucket resources as described in hashicorp/terraform#13882

TODO:

handle update
update acceptance tests
documentation

Supersedes PR in terraform repo: hashicorp/terraform#14802

PS: this is my first attempt at contributing code to Terraform, thank you for your indulgence, all feedback welcome.

pdecat · 2017-06-13T09:28:08Z

I've added tests for update case and everything seems to be working fine without specific handling.

Am I missing anything?

pdecat · 2017-06-13T10:35:04Z

Documentation added,

pdecat · 2017-06-13T10:42:32Z

google/resource_storage_bucket.go

@@ -378,3 +448,135 @@ func flattenCors(corsRules []*storage.BucketCors) []map[string]interface{} {
 	}
 	return corsRulesSchema
 }
+
+func resourceGCSBucketLifecycleCreateOrUpdate(d *schema.ResourceData, sb *storage.Bucket, update bool) error {


update is not used as of now and may be removed. I added it initially as I thought a special treatment was required for update case as opposed to creation.

Sounds reasonable to remove it then

selmanj

Looks really good so far. Giving this review a quick first pass before someone else takes a look.

selmanj · 2017-06-19T20:19:08Z

google/resource_storage_bucket.go

+										Optional: true,
+										Elem:     &schema.Schema{Type: schema.TypeString},
+									},
+									"number_of_newer_versions": {


The api describes this field as 'numNewerVersions' so I suppose it should be 'num_newer_versions' for consistency sake (although I do prefer this name, it's probably better to match the API).

Fair enough, done.

selmanj · 2017-06-19T20:22:22Z

google/resource_storage_bucket.go

@@ -378,3 +448,135 @@ func flattenCors(corsRules []*storage.BucketCors) []map[string]interface{} {
 	}
 	return corsRulesSchema
 }
+
+func resourceGCSBucketLifecycleCreateOrUpdate(d *schema.ResourceData, sb *storage.Bucket, update bool) error {


Sounds reasonable to remove it then

selmanj · 2017-06-19T20:23:07Z

google/resource_storage_bucket.go

+		lifecycle_rules := v.([]interface{})
+
+		if len(lifecycle_rules) > 100 {
+			return fmt.Errorf("At most 100 lifecycle_rule blocks are allowed")


Would be nice to print how many lifecycle_rules were actually found in the error message

selmanj · 2017-06-19T20:34:35Z

google/resource_storage_bucket.go

+
+			if v, ok := lifecycle_rule["condition"]; ok {
+				condition := v.(*schema.Set).List()[0].(map[string]interface{})
+				condition_elements := 0


Rather than counting the condition elements (which might be harder to track later) how about just asserting that the condition map contains at least one expected key?

Actually, correct me if I'm wrong, but can you just say something like

if len(condition) < 1 { // Error here }

Good point! Added a MinItems: 1 on matches_storage_class to ensure it is not empty if specified.

selmanj · 2017-06-19T20:38:31Z

website/docs/r/storage_bucket.html.markdown

+
+* `storage_class` - (Required if action type is `SetStorageClass`) The target [Storage Class](https://cloud.google.com/storage/docs/storage-classes) of objects affected by this Lifecycle Rule. Supported values include: `MULTI_REGIONAL`, `REGIONAL`, `NEARLINE`, `COLDLINE`.
+
+The `condition` block supports:


You might also note that at least one condition map entry is required

Indeed, clarified this part.

paddycarver

This looks great! I only have a couple of nitpicks. Thanks so much for diving into this and for the interest in contributing to Terraform!

paddycarver · 2017-07-04T04:09:29Z

google/resource_storage_bucket.go

+							Type:     schema.TypeSet,
+							Required: true,
+							MaxItems: 1,
+							Set:      resourceGCSBucketLifecycleRuleActionHash,


In theory, I'm not sure this is required, but I doubt there's much harm to including it.

I had to dig a bit to find out the constraints. The XML API doc looks a bit more precise than the JSON API about those facts, but I may be wrongly mixing those two.

From the XML API :

Action Defines the action to be taken, which must contain one and only one specific action element. Required.

cf. https://cloud.google.com/storage/docs/xml-api/put-bucket-lifecycle#request_body_elements

From the JSON API:

A lifecycle management rule, which is made of an action to take and the condition(s) under which the action will be taken.

cf. https://cloud.google.com/storage/docs/json_api/v1/buckets#resource-representations

I meant, specifically, the custom hashing function. :)

paddycarver · 2017-07-04T04:09:54Z

google/resource_storage_bucket.go

+							Type:     schema.TypeSet,
+							Required: true,
+							MaxItems: 1,
+							Set:      resourceGCSBucketLifecycleRuleConditionHash,


Similarly, in theory I don't think this is required, but I don't see much harm in including it.

Same as above, the XML API doc looks a bit more precise than the JSON API about those facts.

XML API:

Condition Defines the condition under which the action will be taken, which must contain at least one specific condition element. Required.

cf. https://cloud.google.com/storage/docs/xml-api/put-bucket-lifecycle#request_body_elements

JSON API:

The condition(s) under which the action will be taken.

cf. https://cloud.google.com/storage/docs/json_api/v1/buckets#resource-representations

paddycarver · 2017-07-04T04:12:23Z

google/resource_storage_bucket.go

+		lifecycle_rules := v.([]interface{})
+
+		len_lifecycle_rules := len(lifecycle_rules)
+		if len_lifecycle_rules > 100 {


I think you can use MaxItems on the resource for this and it will catch this before apply time.

Good point! Done.

paddycarver · 2017-07-04T04:13:00Z

google/resource_storage_bucket.go

+		sb.Lifecycle.Rule = make([]*storage.BucketLifecycleRule, 0, len_lifecycle_rules)
+
+		for _, lifecycle_rule := range lifecycle_rules {
+			lifecycle_rule := lifecycle_rule.(map[string]interface{})


Eeek! This is a particularly confusing bit of shadowing. Any chance we can give this a distinct name?

Indeed, fixed.

paddycarver · 2017-07-04T04:13:46Z

google/resource_storage_bucket.go

+			target_lifecycle_rule := &storage.BucketLifecycleRule{}
+
+			if v, ok := lifecycle_rule["action"]; ok {
+				action := v.(*schema.Set).List()[0].(map[string]interface{})


Check for an empty list here, to avoid panics?

I was relying on the Required schema constraint there but can add the check.

Is that still required with MinItems and MaxItems set to 1?

Added checks on actions list length.

I apologise, you were correct--MaxItems and MinItems would have handled that constraint for you. But the extra check also doesn't hurt.

paddycarver · 2017-07-04T04:14:37Z

google/resource_storage_bucket.go

+			}
+
+			if v, ok := lifecycle_rule["condition"]; ok {
+				condition := v.(*schema.Set).List()[0].(map[string]interface{})


Same here, check for empty list?

Is that still required with MinItems and MaxItems set to 1?

Added checks on conditions list length.

paddycarver · 2017-07-04T04:15:12Z

google/resource_storage_bucket.go

+				condition := v.(*schema.Set).List()[0].(map[string]interface{})
+
+				if len(condition) < 1 {
+					return fmt.Errorf("At least one condition element is required")


I think we could use MinItems to catch this prior to apply time.

Done. Also, I forgot the same check on the action.

I think I remember why I implemented this check differently at first by incrementing a counter: 545e5b5

From the XML API doc:

Condition Defines the condition under which the action will be taken, which must contain at least one specific condition element. Required.

cf. https://cloud.google.com/storage/docs/xml-api/put-bucket-lifecycle#request_body_elements

…erface

…cks, and that at least one condition element has to be defined

…w_lifecycle_rule

…ed, use MinItems in schema to enforce

pdecat · 2017-07-17T09:57:35Z

Hi, I believe all issues were addressed.

paddycarver

LGTM with the changes 👍 Great work @pdecat, thanks for tackling this. Sorry for the delay on finishing my review--I was off on vacation.

@selmanj anything further to add before we get this merged?

selmanj · 2017-07-25T23:10:49Z

Looks good to me!

…fecycle Add provider/google/google_storage_bucket lifecycle interface

Add provider/google/google_storage_bucket lifecycle interface

ghost · 2020-03-29T14:22:09Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

pdecat mentioned this pull request Jun 12, 2017

[WIP] Add provider/google/google_storage_bucket lifecycle interface hashicorp/terraform#14802

Closed

3 tasks

pdecat force-pushed the f-google-storage-bucket-lifecycle branch from 6b20178 to 47ef63e Compare June 13, 2017 09:25

pdecat changed the title ~~[WIP] Add provider/google/google_storage_bucket lifecycle interface~~ Add provider/google/google_storage_bucket lifecycle interface Jun 13, 2017

pdecat commented Jun 13, 2017

View reviewed changes

rileykarson mentioned this pull request Jun 13, 2017

feature request: provider/google/google_storage_bucket lifecycle interface #84

Closed

rileykarson added the enhancement label Jun 14, 2017

selmanj assigned selmanj and unassigned selmanj Jun 19, 2017

selmanj self-requested a review June 19, 2017 17:29

selmanj reviewed Jun 19, 2017

View reviewed changes

pdecat force-pushed the f-google-storage-bucket-lifecycle branch from dda7b77 to e1044e3 Compare June 20, 2017 17:45

selmanj requested a review from paddycarver June 22, 2017 22:09

pdecat force-pushed the f-google-storage-bucket-lifecycle branch 3 times, most recently from d0b46ef to 1dcb414 Compare June 29, 2017 06:05

paddycarver suggested changes Jul 4, 2017

View reviewed changes

pdecat force-pushed the f-google-storage-bucket-lifecycle branch from 1dcb414 to 3381e9f Compare July 4, 2017 11:13

pdecat added 11 commits July 17, 2017 11:46

[WIP] Add provider/google/google_storage_bucket lifecycle interface

7dc4c9d

Add documentation provider/google/google_storage_bucket lifecycle int…

7676453

…erface

Rename number_of_newer_versions as num_newer_versions

accb665

Remove useless update param

dfd3bde

Print number of lifecycle rules passed if there are too many (> 100)

ec145fe

Simplify minimum condition elements assertion

28219ef

Clarify documentation regarding the required action and condition blo…

05d2567

…cks, and that at least one condition element has to be defined

Fix acceptance tests

da1c673

Avoid shadowing lifecycle_rule variable, rename unconverted one to ra…

8bf132d

…w_lifecycle_rule

Implement limit on lifecycle_rules using MaxItems in schema

40fad4b

Action and condition blocks of a lifecycle_rule block are both requir…

c43b8b4

…ed, use MinItems in schema to enforce

Add checks on actions and conditions lists lengths

a0ba61a

pdecat force-pushed the f-google-storage-bucket-lifecycle branch from 326e42e to a0ba61a Compare July 17, 2017 09:47

paddycarver approved these changes Jul 25, 2017

View reviewed changes

paddycarver merged commit 775d931 into hashicorp:master Jul 25, 2017

negz pushed a commit to negz/terraform-provider-google that referenced this pull request Oct 17, 2017

Merge pull request hashicorp#6 from pdecat/f-google-storage-bucket-li…

66f8d8f

…fecycle Add provider/google/google_storage_bucket lifecycle interface

pdecat deleted the f-google-storage-bucket-lifecycle branch March 7, 2018 09:00

chrisst referenced this pull request in chrisst/terraform-provider-google Nov 9, 2018

Merge pull request #6 from pdecat/f-google-storage-bucket-lifecycle

3c5445d

Add provider/google/google_storage_bucket lifecycle interface

ghost locked and limited conversation to collaborators Mar 29, 2020


		* `storage_class` - (Required if action type is `SetStorageClass`) The target [Storage Class](https://cloud.google.com/storage/docs/storage-classes) of objects affected by this Lifecycle Rule. Supported values include: `MULTI_REGIONAL`, `REGIONAL`, `NEARLINE`, `COLDLINE`.

		The `condition` block supports:

Add provider/google/google_storage_bucket lifecycle interface #6

Add provider/google/google_storage_bucket lifecycle interface #6

Conversation

pdecat commented Jun 12, 2017 • edited Loading

pdecat commented Jun 13, 2017

pdecat commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selmanj 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

paddycarver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdecat Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdecat Jul 4, 2017 • 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

pdecat Jul 4, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdecat Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

pdecat commented Jul 17, 2017

paddycarver left a comment

Choose a reason for hiding this comment

selmanj commented Jul 25, 2017

ghost commented Mar 29, 2020

pdecat commented Jun 12, 2017 •

edited

Loading

pdecat Jul 4, 2017 •

edited

Loading

pdecat Jul 4, 2017 •

edited

Loading

pdecat Jul 4, 2017 •

edited

Loading

pdecat Jul 4, 2017 •

edited

Loading