remove "id" fields from schema definitions

[jbardin]

The "id" field is built-in and not required in the schema. Future
updates to helper/schema will cause these to fail validation.

[radeksimko]

For the ones which are Optional, not just Computed, there's possibly a bit more cleanup we can do - do you mind searching for "id" and replace d.Set("id", ...) w/ d.SetId() and d.GetOk("id") w/ d.Id()?

[jbardin]

Good idea @radeksimko. I updated the calls throughout, and found a few more top-level "id" fields that weren't caught by the unit tests.

I also removed some SetPartial calls to "id", which shouldn't be required since SetId is being called outside of Partial.

[radeksimko]

Thanks for making those changes - I just ran a few experiments to verify @paddycarver 's thoughts about Optional fields he mentioned in hashicorp/terraform-provider-google#399 (review) and I have to admit he's right.

We can't just remove them 😢 , because in some cases they may be working (if the CRUD is correctly using d.Id() to get it and d.SetId() to set it). At this stage we'd break those data sources, e.g. aws_eip:

data "aws_eip" "e" {
  id = "eipalloc-0128413b" # This is currently valid and works just fine
}

I'm thinking my validation may be more aggressive than I thought. I'm pondering with an idea of excluding data sources from the validation or allowing Optional id fields to exist if they're also Deprecated or Removed.

I'm more inclined to the latter (slowly allow phasing out) because it can be confusing to work with this field in the CRUD. It's not immediately obvious d.Id() and d.SetId() are dedicated getters and setters.

What do you think?

[azman0101]

It seems the documentation doesn't mention built-in id attribute https://www.terraform.io/docs/providers/aws/d/iam_server_certificate.html#attributes-reference

[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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!