- Notifications
You must be signed in to change notification settings - Fork1.1k
fix: use resource_id directly for coder_metadata association#18300
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Change-Id: Ibda8f39393b6df90b98bc82e2a005a506830ce00Signed-off-by: Thomas Kosiewski <tk@coder.com>
ae3960b to7e0c0a2Compare
johnstcn left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we are at a point where it would make sense to start breaking upConvertState. This metadata association piece seems like a good candidate to start with and have individual unit tests around.
| case0: | ||
| // If we couldn't find by ID, fall back to graph traversal | ||
| logger.Warn(ctx,"coder_metadata resource_id not found, falling back to graph traversal", | ||
| slog.F("resource_id",attrs.ResourceID), | ||
| slog.F("metadata_address",resource.Address)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As far as I can tell, this warn log will just end up in the server log and won't be especially visible to template authors.
I can see an argument for erroring out here, but the larger argument against is that it would most likely break existing templates. How do we feel about this?
| default: | ||
| // Multiple resources with same ID - this creates ambiguity | ||
| logger.Warn(ctx,"multiple resources found with same resource_id, falling back to graph traversal", | ||
| slog.F("resource_id",attrs.ResourceID), | ||
| slog.F("metadata_address",resource.Address), | ||
| slog.F("matching_labels",foundLabels)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Again, I can see an argument for erroring here to surface the misconfiguration but worry about the potential for breaking existing templates.
| continue | ||
| } | ||
| } | ||
| ifattachedResource==nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In what situation would we encounter this? It's not covered in existing or new tests.
| resourceLabel:=convertAddressToLabel(resource.Address) | ||
| varattachedNode*gographviz.Node | ||
| for_,node:=rangegraph.Nodes.Lookup { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
At this stage, it would make sense to extract the "metadata association" piece out and test it separately.
| resource"null_resource""example" {} | ||
| resource"coder_metadata""example" { | ||
| resource_id="non-existent-id" |
ethanndicksonAug 13, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should have another test to check that it falls back to graph traversal when theresource_id is set tocoder_agent.whatever.id. I've seen that pattern in many customer templates in the past.
spikecurtis commentedAug 13, 2025
Is there an issue that describes the problem this is intending to solve? It worries me that we're using resource IDs to identify the resource the metadata refers to when resource IDs are not unique. I think it would be better to deprecate the resource ID and instead use something unique like (module, resource_type, name). |
sreya commentedAug 13, 2025
Yeah the problem is the metadata gets assigned to the wrong terraform resource. So on the workspace page, metadata you intend to be displayed alongside the agent is instead hidden on a random resource that you have to open the side bar to see. This one is working as intended but to give an idea of what i'm describing  |
Uh oh!
There was an error while loading.Please reload this page.
Previously, coder_metadata resources would associate with the wrong Terraform resource because the implementation relied on graph traversal instead of using the explicit resource_id attribute.
This fix:
This ensures that metadata is correctly associated with the intended resource, even when there are complex dependencies in the Terraform configuration.
Fixes the issue where coder_metadata would incorrectly associate with resources it references in its values rather than the resource specified in resource_id.
Closes#18301
Closescoder/terraform-provider-coder#306