Conversation
|
/review |
There was a problem hiding this comment.
Correct fix for nested @defaults expansion; one defensive gap in the printer
| hasEntrypoints := block != nil && len(block.Entrypoints) > 0 | ||
| if !hasEntrypoints { | ||
| res.WriteString(name) | ||
| } |
There was a problem hiding this comment.
🟡 warning — If hasEntrypoints is true but every entrypoint is skipped (no matching checksum data in m), the returned string will be empty — the resource name was suppressed and nothing took its place. Previously this path still produced the resource name.
Consider falling back to the resource name when nothing was printed:
hasEntrypoints := block != nil && len(block.Entrypoints) > 0
if !hasEntrypoints {
res.WriteString(name)
}
// ... entrypoint loop ...
// after the loop:
if hasEntrypoints && first {
// all entrypoints were skipped, fall back to resource name
res.WriteString(name)
}| if idx := strings.LastIndex(typeName, "."); idx >= 0 { | ||
| label = parentLabel + typeName[idx:] |
There was a problem hiding this comment.
🔵 suggestion — Using LastIndex means that for a type like gcp.project.pubsubService.topic, only .topic is appended. This works well for deep hierarchies, but if two sibling resources happen to share the same final segment (e.g., both aws.ec2.securityGroup.rule and aws.vpc.securityGroup.rule exist), labels could collide under the same parent. The current naming conventions make this unlikely, but a comment noting the assumption would help future readers.
| for i := range code.Blocks { | ||
| // Use index-based loop instead of range so that blocks appended by | ||
| // expandResourceFields (nested @defaults) are visited in the same pass. | ||
| for i := 0; i < len(code.Blocks); i++ { |
There was a problem hiding this comment.
🔵 suggestion — Good fix — range captures slice length once, so blocks appended by expandResourceFields were silently skipped. The index-based loop is the right approach. Consider adding a guard against run-away expansion (e.g., a max-iteration cap) just in case future .lr changes introduce a cycle:
const maxBlocks = 10_000
for i := 0; i < len(code.Blocks) && i < maxBlocks; i++ {There was a problem hiding this comment.
does not adress the root problem and is a safeguard against a very low expansion loop.
When querying resources with nested @defaults (e.g., gcp.project.pubsub.topic{*}), nested resource fields displayed as raw resource IDs instead of their declared default fields. For example, a topic's
messageStoragePolicywould show its internal ID rather than expanding to showallowedPersistenceRegionsas declared in the .lr definition.In addition i changed the resource display and labels as they were for me personally quite clubbering some things up.
This is purely for user output. So please review if its up to the liking. Or if it takes some logic for the user.
We now trust more the named fields instead of the full path.
if
messageStoragePolicy.allowedPersistenceRegionsis used in defaults this is unchangedAfter
Before