Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements naming convention changes for multi-registry manifests as specified in issue #1197. The changes standardize the way registries are identified and referenced across the Weaver codebase.
Changes:
- Renames
registry_manifest.yamltomanifest.yaml(with backward compatibility for the old name) - Introduces
SchemaUrltype to standardize registry identification using OTel schema URL format - Replaces separate
name,version, andschema_base_urlfields with unifiedschema_urlfield - Renames
resolved_schema_urltoresolved_schema_urifor consistency - Updates all test data and references throughout the codebase to use the new naming conventions
Reviewed changes
Copilot reviewed 49 out of 50 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/weaver_semconv/src/schema_url.rs | New SchemaUrl type for URL-based registry identification with name/version extraction |
| crates/weaver_semconv/src/manifest.rs | Updated RegistryManifest and Dependency with schema_url field, deprecated old fields |
| crates/weaver_semconv/src/registry_repo.rs | Updated RegistryRepo to use schema_url, added legacy manifest file support |
| crates/weaver_semconv/src/registry.rs | Updated registry loading to use SchemaUrl |
| crates/weaver_resolver/* | Updated resolver to work with schema_url instead of registry_id |
| crates/weaver_resolved_schema/src/v2/* | Updated V2 schema to use SchemaUrl, removed registry_url field |
| crates/weaver_forge/src/v2/registry.rs | Updated ForgeResolvedRegistry to use schema_url |
| crates/weaver_live_check/* | Updated to use boxed V2 types and schema_url |
| tests/* | Updated test code to use new API with schema_url |
| Test data files | Updated manifest files to use new schema_url format |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crates/weaver_resolver/data/circular-registry-test/registry_b/registry_manifest.yaml
Outdated
Show resolved
Hide resolved
crates/weaver_resolver/data/registry-test-published-1/registry/registry_manifest.yaml
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1202 +/- ##
======================================
Coverage 80.3% 80.4%
======================================
Files 109 110 +1
Lines 8855 8984 +129
======================================
+ Hits 7113 7225 +112
- Misses 1742 1759 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…registry_manifest.yaml Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…/registry_manifest.yaml Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…into manifest-renames
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 49 out of 50 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 51 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jsuereth
left a comment
There was a problem hiding this comment.
Overall looks good!
- Should we have some kind of "TODO" tracker to remove the backwards compatibility support after version X or some such?
- How will we add warnings around supporting legacy manifest file names, or legacy structure for name/version? We may want to keep some flag on a Manifest we can later producer "diagnostic" warnings from, post serde.
7cc29e4 to
2ec64fa
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 51 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
I've updated the PR to:
Let's chat about back-compat policy on the SIG call tomorrow |
jsuereth
left a comment
There was a problem hiding this comment.
Overall, almost ready to merge, but a bunch of rust-related cleanups.
Fixes #1197
Renames:
registry_manifest.yamlis now deprecated (but supported), usemanifest.yamlinsteadschema_urlis now used consistently in resolved schema and manifest instead of registry_id, registry_url, ... . It can be a source of the registry contents, but does not have to be. When specifying dependencies, it's allowed to passregistry_pathin addition toschema_url. This way, schema url is essentially a combination of name and version in a certain format.resolved_schema_url->resolved_schema_uriChanges should be backward compatible, old properties of
RegistryManifestandDependencyare supported, but deprecated