helper/schema: Allow identification of a new resource in update func

[radeksimko]

TL;DR

This is to avoid situations where we call Update function right from the Create function and the Update function has in some situations no way to differentiate between a new resource and resource that's being updated.

e.g. in KMS I'm enabling/disabling keys in the Update function, but I don't want to call the enable/disable key API at all if I know that the key is enabled by default once created.

Why not ____

d.HasChange ?

You could say that d.HasChange() may cover such situations, but if we have

            "is_enabled": &schema.Schema{
                Type:     schema.TypeBool,
                Optional: true,
                Default:  true,
            },

and is_enabled not present in the HCL template then

d.HasChange("is_enabled")

is always true when creating a new resource. d.GetChange reports false => true which makes it impossible to differentiate between creation & real update from is_enabled=false to is_enabled=true.

Strict separation between Create & Update?

Also you could say that we just may not call Update from Create and make these strictly separated, but that may (in some cases) involve a lot of duplication purely because some attributes can only be set from Update APIs. This approach would therefore make the code much more verbose.

Just always call update?

Because more API calls cost more time - in case like KMS, where we need to confirm the state has been properly propagated it makes a huge difference - even minutes. Each API call also counts toward the limit and we should make Terraform efficient enough to not throttle any APIs.

[phinze]

Is there any reason to not just have IsNewResource() check Id() == "" directly rather than keeping the boolean struct member?

[radeksimko]

That is a good point. Making IsNewResource() just an "alias" for Id() == "" is much cleaner approach. I will update the PR.

[radeksimko]

In fact it doesn't seem to be equivalent, tests are failing.

It's been over a week since I last had my head in this part of codebase, so I don't quite know why, but I'll find out and post an explanation here.

[radeksimko]

So this is because if we have func Create() { return Update() }

then the Create func already sets id for the resource which means that Id() == "" in Update() function will never be true.

Resources with ForceNew: true are also covered:
https://github.com/TimeIncOSS/terraform/blob/f-schema-new-resource/helper/schema/resource.go#L119-L127

---

Looking at the code also makes me wonder why do we explicitly call d.SetId("") in most Delete() functions in resources, if we're resetting it here anyway. It seems superfluous.

[phinze]

Ahhh sure that makes sense. This implementation works then.

[phinze]

This seems reasonable at first pass - question on the implementation details, but thanks for the solid explanation!

[radeksimko]

@phinze Let me know if you're happy with the explanation. It also doesn't seem very elegant to me to use that extra struct member, but I can't think of any better solution.

[phinze]

Yep that makes sense - thanks for digging in.

Let me think about this one a bit more. I'm still wondering if the "Create calls Update calls Read" pattern is worth defending with extra code here rather than eating the verbosity of the "Strict separation" route you mentioned.

[josephholsten]

@phinze is this waiting on anything? looks ready-to-go and worth-having.

[phinze]

Just took a second look here - this LGTM @radeksimko.

[ghost]

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.