Repository navigation
fix(projectloader): unescape service names in unsupported attribute warnings - #14380
Huang-404-Q wants to merge 2 commits into
Conversation
…arnings compose-go escapes a literal dot in a path segment as an internal sentinel, and splitServicePath returned the raw segment. A service named "my.service" was therefore reported as "my👻service" in warnings. Unescape the segment with tree.Path(...).String(). Signed-off-by: huang <13165998033@163.com>
bfc2a16 to
a9a3808
Compare
ndeloof
left a comment
ndeloof
left a comment
There was a problem hiding this comment.
Thanks for the fix, LGTM. I checked that the new subtest fails without the change (Service: "my👻service") and passes with it, and that golangci-lint is clean on the package.
Two optional suggestions below; I validated both locally (package tests and lint pass with them applied).
| parts := path.Parts() | ||
| if len(parts) >= 2 && parts[0] == "services" { | ||
| return parts[1], strings.Join(parts[2:], ".") | ||
| return tree.Path(parts[1]).String(), strings.Join(parts[2:], ".") |
There was a problem hiding this comment.
Nit: the project-scoped branch below returns path.String() (unescaped), whereas the remaining service path is still joined from the raw parts, so a dotted key deeper in the path would keep the 👻 sentinel. I could not find a reachable case today, but a future generic finding under a free-key map would leak it the same way. Unescaping both keeps the two branches consistent:
| return tree.Path(parts[1]).String(), strings.Join(parts[2:], ".") | |
| return tree.Path(parts[1]).String(), tree.Path(strings.Join(parts[2:], ".")).String() |
There was a problem hiding this comment.
Applied in 926eea4: both branches now unescape through tree.Path(...).String(), so a dotted key deeper in the path can't leak the sentinel.
| mode: replicated | ||
| `) | ||
| assertFindings(t, got, []findingKey{{Service: "my.service", Path: "deploy.mode"}}) |
There was a problem hiding this comment.
Optional: the value-conditional branch (ports[].mode: host) also receives service from splitServicePath, so it benefits from this fix but is not covered with a dotted name. Extending the subtest covers both reporting paths:
| mode: replicated | |
| `) | |
| assertFindings(t, got, []findingKey{{Service: "my.service", Path: "deploy.mode"}}) | |
| mode: replicated | |
| ports: | |
| - target: 80 | |
| published: "8080" | |
| protocol: tcp | |
| mode: host | |
| `) | |
| assertFindings(t, got, []findingKey{ | |
| {Service: "my.service", Path: "deploy.mode"}, | |
| {Service: "my.service", Path: "ports[80/tcp].mode"}, | |
| }) |
There was a problem hiding this comment.
Extended in 926eea4: the dotted-service subtest now also covers the ports[].mode reporting path (ports[80/tcp].mode). Package tests and gofmt pass.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Unescape the remaining path in both branches and cover the ports[].mode reporting path with a dotted service name. Signed-off-by: huang <13165998033@163.com>
What
compose-go represents a literal dot inside a key segment with an internal sentinel (
👻) so thattree.Pathcan split on real separators.splitServicePathreturnedparts[1]raw, so a legal service name containing a dot was passed toapi.UnsupportedAttribute.Servicewith the sentinel still embedded:Expected:
service "my.service": ...Fix
Unescape the segment with
tree.Path(parts[1]).String(). The remaining dotted path is unchanged.Tests
Added a subtest to
TestUnsupportedAttributesusingmy.serviceand asserting the reported service ismy.service, plus the existingweb/deploy.modecontrols.Before the fix the new subtest fails with
Service: "my👻service"; after it passes and the fullTestUnsupportedAttributessuite is green.