Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove "id" fields from schema definitions #1626

Merged
merged 3 commits into from
Oct 26, 2017
Merged

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Sep 8, 2017

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

The "id" field is built-in and not required in the schema. Future
updates to helper/schema will cause these to fail validation.
@jbardin jbardin requested a review from radeksimko September 8, 2017 19:57
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?

Replace Set and Get with "id" keys with SetId and Id, and remove
redundant uses of these.

Remove a few more stray "id" fields in schemas. The few remaining are
not at the top level and don't represent "resources" in their own.
@jbardin jbardin force-pushed the jbardin/no-id-fields branch from d244c1e to 37da2b0 Compare September 11, 2017 13:22
@jbardin
Copy link
Member Author

jbardin commented Sep 11, 2017

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
Copy link
Member

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?

@radeksimko radeksimko added the enhancement Requests to existing resources that expand the functionality or scope. label Sep 27, 2017
@azman0101
Copy link

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
Copy link

ghost commented Apr 3, 2020

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!

@ghost ghost locked and limited conversation to collaborators Apr 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants