Static graph icons + default icon#12396
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Unit Tests 2 files ± 0 456 suites ±0 7m 12s ⏱️ -14s Results for commit 23fd4ff. ± Comparison against base commit 8b19616. This pull request removes 12 and adds 41 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12396 +/- ##
==========================================
- Coverage 53.70% 53.41% -0.30%
==========================================
Files 761 762 +1
Lines 49771 50246 +475
==========================================
+ Hits 26729 26837 +108
- Misses 20544 20882 +338
- Partials 2498 2527 +29 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
c036089 to
32810da
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends the resource-type icon work to support static (modeled) application graphs (no control plane) and ensures every registered resource type gets an iconHash via default-hash substitution when no SVG is supplied. It also wires the default-icon fallback through the icon endpoint and the runtime graph’s icons map so consumers can resolve bytes for defaulted types.
Changes:
- Add a CLI/control-plane shared
deploy/manifesticon catalog (embedded default SVG + embedded per-type SVGs) with helpers to look up icons and default hashes. - Default-hash substitution at resource-type registration time (when
iconis absent), plus icon-endpoint fallback to serve embedded default bytes when the stored hash matches the default. - Modeled graph now emits
iconHashper node and (optionally) aniconsmap viarad app graph <bicep> --include-icons, plus updated tests/docs.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| specs/003-resource-type-icons/plan.md | Updates Slice 2 plan/status and documents delivered changes for default icon substitution + static graph icons. |
| pkg/ucp/integrationtests/resourceproviders/testdata/resourcetype_v20231001preview_responsebody.json | Updates integration test fixture to include iconHash in resource type summary response. |
| pkg/ucp/integrationtests/resourceproviders/testdata/resourcetype_v20231001preview_list_responsebody.json | Updates list fixture to include iconHash. |
| pkg/ucp/integrationtests/resourceproviders/testdata/resourcetype_manifest_responsebody.json | Updates manifest fixture to include iconHash. |
| pkg/ucp/integrationtests/resourceproviders/testdata/resourcetype_manifest_list_responsebody.json | Updates manifest list fixture to include iconHash. |
| pkg/ucp/integrationtests/resourceproviders/summary_test.go | Asserts default-hash substitution is reflected in provider summary results. |
| pkg/ucp/initializer/service.go | Substitutes product default icon hash when registering a type without icon bytes (built-in manifest path). |
| pkg/ucp/initializer/service_test.go | Updates tests to assert default-hash substitution behavior. |
| pkg/ucp/frontend/controller/resourceproviders/getresourceprovidersummary.go | Removes FR references from comments; retains includeIcons behavior and hash-only semantics. |
| pkg/ucp/frontend/controller/resourceproviders/geticon.go | Serves embedded default icon bytes when stored hash matches default but stored bytes are nil. |
| pkg/ucp/frontend/controller/resourceproviders/geticon_test.go | Adds coverage for default-hash fallback serving embedded bytes. |
| pkg/ucp/datamodel/icon_validation.go | Clarifies/modernizes comments for icon validation without spec-number coupling. |
| pkg/ucp/api/v20231001preview/resourcetype_conversion.go | Substitutes default icon hash when ARM PUT registration omits icon bytes (conversion path). |
| pkg/ucp/api/v20231001preview/resourcetype_conversion_test.go | Updates conversion tests to assert default-hash substitution. |
| pkg/corerp/frontend/controller/applications/v20250801preview/graphicons.go | Switches icon enrichment to use shared SplitResourceType and applies default-hash fallback per node. |
| pkg/corerp/frontend/controller/applications/v20250801preview/graphicons_test.go | Updates tests for attachIconHashes/default fallback behavior (but needs renames for clarity). |
| pkg/corerp/frontend/controller/applications/v20250801preview/graph_util.go | Adds v20250801preview-typed graph computation pipeline (version-local model typing). |
| pkg/corerp/frontend/controller/applications/v20250801preview/getgraph.go | Uses v20250801preview-typed pipeline and fills icons map with embedded default bytes when appropriate. |
| pkg/corerp/frontend/controller/applications/getgraph.go | Clarifies that v20250801preview owns its own end-to-end graph pipeline (no shared versioned model types). |
| pkg/cli/graph/modeled.go | Adds per-node iconHash resolution from embedded manifest and optional icons map for modeled graphs. |
| pkg/cli/graph/modeled_test.go | Updates modeled-graph tests for new signature + includeIcons behavior. |
| pkg/cli/cmd/app/graph/graph.go | Extends --include-icons semantics to modeled graphs and plumbs the flag through Runner. |
| docs/architecture/application-graph.md | Documents default icon substitution and static graph icon behavior (but diagram needs to keep iconHash optional). |
| deploy/manifest/icons.go | New embedded icon catalog: default icon + built-in icons + helpers (Lookup/Default/DefaultHash/IsDefault/SplitResourceType). |
| deploy/manifest/icons_test.go | Adds tests pinning default icon invariants and lookup behavior. |
| deploy/manifest/default-icon.svg | Adds canonical product default icon SVG embedded into binaries. |
brooke-hamilton
left a comment
brooke-hamilton
left a comment
There was a problem hiding this comment.
A couple of minor doc/comment nits — nothing blocking. Nice, well-tested change.
|
Recommend looking into these Copilot findings regarding validation of SVG data against malicious code: Only partially.
It is not a complete SVG sanitizer. It permits |
12bd24e to
b11e1b1
Compare
Addressed these issues in the validation. |
b11e1b1 to
407ddb2
Compare
407ddb2 to
9009c41
Compare
|
Some more findings, please check to see if these can also be addressed: The PR now rejects
Separately, the earlier integrity gap remains: gathered SVG bytes are still not re-hashed and compared with their advertised |
willtsai
left a comment
willtsai
left a comment
There was a problem hiding this comment.
Suggested tightening SVG validation to reject SMIL animation elements that can mutate checked attributes after validation.
willtsai
left a comment
willtsai
left a comment
There was a problem hiding this comment.
thanks for addressing the icon validation feedback.
Radius functional test overviewClick here to see the test run details
Test Status⌛ Building Radius and pushing container images for functional tests... |
Summary
Static (modeled) graph now emits iconHash and a matching icons map from the CLI-embedded manifest — rad app graph app.bicep renders icons with no control plane.
Default-icon substitution at registration time: types registered without an SVG get the product default hash stamped on the record
Graceful degradation — "icon absence is not an error": broken embedded asset logs and continues, callers set icon Hash to nil.
Note: static graphs can have only default icons for non default types, since we need control plane to register the tyep with an icon. All default.yaml types should render with icons in both static and run time graphs.
Fixes #12339
How to test
Contsruct an app.bicep with RRTs and and built in types and some types of your own.
rad app graph /Users/nithya/radius/my-radius-recipes/deploy/app.bicep should produce app-graph.json that includes icons.