Remove 'id' fields from schemas

[radeksimko]

I'm happy to remove all vendor changes and make a separate PR if necessary.

I also ran all provider acceptance tests on this branch in TC and nothing suspicious came up.

[catsby]

Can you elaborate on the "why" here?

[radeksimko]

Because IDs are always treated separately (via d.SetId() & d.Id()) + ID has a special meaning (non-empty ID == existence of the resource) and it's always present in the schema so it should never be explicitly specified there.

[radeksimko]

I will look into the test failures.

[paddycarver]

My take: rather than dropping the fields entirely, we should rename them to <whatever>_id. This allows us to store the full API-side ID without being bound to it in Terraform. For a few reasons (import, versioning, etc.) it's nice to have some flexibility in how Terraform uses the ID internally. There are also some cases where the API IDs of things differ even for the same resource.

My approach to this to date has been discouraging users from using .id as a meaningful value and making sure whatever .id would have been is exposed as a field, to surface that value in a way users can rely on.

[radeksimko]

@paddycarver I agree that removing the id from google_project was a bit too aggressive, apologies and thanks for pointing this out! I can modify the PR to keep it there. I didn't realize it's somehow working because the CRUD code used d.Id() to get the value or d.SetId() to set it.

I still believe it's confusing to keep id in the schema, but I'm willing to tune the validation to allow such fields to exist if they're Deprecated or Removed.

That said I don't believe there's any harm in removing Computed-only fields and/or associated d.Set("id"). The d.Set("id") does literally nothing and d.Get("id") always returns empty string.

d.SetId() is the only way to set ID and d.Id() to get it. So if d.SetId() was previously used to set an ID, then this will keep working, just the ID won't be explicitly visible in the schema, which the user doesn't really care about.

Let me know what you think.

[paddycarver]

I think we're largely in agreement here, @radeksimko. Project should be fine now--it was deprecated with 0.9.0 9 months ago, if people haven't updated yet, they're not going to. And besides, with 1.0.0 about to go out, may as well lump in that break.

Only thing I'd ask different on this one: instead of removing the .id properties that are Computed-only, let's rename them to e.g. certificate_id, like google_project.id became google_project.project_id. That way we expose that info to users that want it.

Just pushed a commit that makes the suggested changes. Tagging @radeksimko for a final thumbs up, and we'll merge it.

[radeksimko]

@paddycarver I clearly can't approve my own PR, so I'm sending 👍 your way via this comment. 😄

[radeksimko]

Totally nitpick, but I guess we can leave out the extra whitespace here.

[catsby]

Throwing my 👍 in there

[paddycarver]

If everyone's OK with it, I'd like to hold off on merging this until the dust settles on 1.0.0.

[paddycarver]

Merging this now. This should have no user-visible change; before this, the id attribute as well as the actual id are getting set to the same value, and from what I can tell, it's the value set using d.SetId. So I'm not considering this a BC, as all user configs should continue working just as they were, and all these fields are computed, so it's not like users could be trying to set them in configs, anyways.

[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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!