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

FAIL: TestProvider: id is a reserved field name for a resource with latests github.com/hashicorp/terraform #353

Closed
cthiel opened this issue Aug 24, 2017 · 12 comments
Assignees
Labels

Comments

@cthiel
Copy link

cthiel commented Aug 24, 2017

I'm building & testing terraform-provider-google with the latests github.com/hashicorp/terraform and ran into this test failure:

--- FAIL: TestProvider (0.01s)
provider_test.go:26: err: 5 errors occurred:
* resource google_project: id is a reserved field name for a resource
* resource google_compute_target_https_proxy: id is a reserved field name for a resource
* resource google_compute_target_http_proxy: id is a reserved field name for a resource
* resource google_compute_url_map: id is a reserved field name for a resource
* resource google_compute_ssl_certificate: id is a reserved field name for a resource

Just removing the id entries from the schema for the respective resources fixes the tests, but I'm unsure about the side-effects this might cause.

@cthiel
Copy link
Author

cthiel commented Aug 24, 2017

This doesn't seem to be happening with the vendored version of hashicorp/terraform in https://github.com/terraform-providers/terraform-provider-google/tree/master/vendor/github.com/hashicorp/terraform

@selmanj
Copy link
Contributor

selmanj commented Aug 24, 2017

Looks like the warning code was introduced here: hashicorp/terraform#15522

@paddycarver paddycarver self-assigned this Aug 24, 2017
@paddycarver
Copy link
Contributor

This appears to have been introduced into this repo in #302. Weirdly, this seems to pass in our CI? It also passes for me locally. Can you let us know what revision you're building under and what command you're using, @cthiel?

That aside, those resources are subtly broke at the moment until those things are fixed. I think, for some resources, the breakage isn't really material, but for others it may make a difference. From my understanding, resources that have an id field will always return the zero value as the value for that field, seeing it as unset even if the user specifies something in their config.

@cthiel
Copy link
Author

cthiel commented Aug 28, 2017

I'm building against hashicorp/terraform master as of last week, ie not using the vendored version (at all). You should be able to update the vendored version using govendor and switch to master instead of the 0.10.0 tag. (Sorry I don't have commands ready to reproduce this easily.)

@paddycarver
Copy link
Contributor

paddycarver commented Aug 28, 2017

So you're explicitly ignoring the vendored version? Can I ask how, and why?

The main reason I ask is I seem to be unable to reproduce this.

[EDIT] Looks like id wasn't explicitly added until hashicorp/terraform#15695, and isn't vendored yet. govendor update github.com/hashicorp/terraform/config does indeed force the error, and I think we can fix this without too much pain/confusion--one of the fields is marked as Removed already, and the others are Computed and not user-specifiable, meaning they're redundant with SetId. I am still interested in/curious why @cthiel is building against tip instead of what's vendored, as that's not guaranteed to work (as this shows) and shouldn't be necessary for most people. [/EDIT]

@cthiel
Copy link
Author

cthiel commented Aug 28, 2017

This is mostly due to the fact that the build system that I'm using is discouraging vendored dependencies. Also, it seems risky to have terraform-providers/terraform-provider-google vendor much and then diverge from master, which then later requires more work to be done to re-base and integrate, like this bug is showing.

(I'm happy to explain in more detail via mail.)

@paddycarver
Copy link
Contributor

Hey @cthiel, thanks for the clarification. While this specific instance is something we're definitely interested in fixing, as it represents code that's broken in the vendored directory, I just want to be clear that we explicitly do not track master of all the dependencies, and builds that don't use our pinned, vendored dependencies are likely to fail, possibly in destructive and surprising ways. They're explicitly not supported.

As for diverging, we don't really ever diverge from any of our dependencies, that I know of. But we do lag. We have so many providers, each with so many dependencies, that keeping all of them working against master all the time is an unrealistic goal. So we update dependencies as they get bug fixes and improvements that are used in the provider, making the changes necessary at that time.

That being said, because this specific change doesn't introduce the break, it just exposes it, I'm definitely interested in getting these resources updated to work correctly.

@cthiel
Copy link
Author

cthiel commented Aug 28, 2017

Sure, thanks for the clarifications and looking into fixing the provider!

@radeksimko
Copy link
Member

From my understanding, resources that have an id field will always return the zero value as the value for that field, seeing it as unset even if the user specifies something in their config.

This certainly shouldn't be happening. The intention of the extra check I introduced a while back was only to force providers to clean up the schema by removing fields which have no effect, id being one of them.

https://github.com/hashicorp/terraform/blob/67ceb1ab07acd55ebc0d7b6efb39019e73fd1888/helper/resource/map.go#L79

Based on ^ that the ID should always be set in the state, if d.SetId() is called. If it isn't called then the resource is more significantly broken already, b/c much of the code relies on non-empty ID being the guarantor of resource existence.

@paddycarver
Copy link
Contributor

@radeksimko this is my understanding based on hashicorp/terraform#10425 (comment) Basically, your check didn't break things, just exposed things that were already broken. So even though d.SetId and d.Set("id") were both called, d.Get("id") returned the empty string--though d.GetId would hypothetically work. Basically, it's d.Get not the ID writing that's broken when id is a field. If memory serves.

But the upshot of this is that we're using a reserved name as a field, we shouldn't be, and the new check doesn't break code, it just tells us code we broke long ago is broken. :) I'll open PRs to migrate off id and onto something not reserved.

@paddycarver
Copy link
Contributor

All uses of id should be gone from the provider now, so I'm going to close this out. Feel free to reopen if it crops back up again!

luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this issue May 21, 2019
@ghost
Copy link

ghost commented Mar 30, 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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

5 participants