Add weaver registry package command#1254
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1254 +/- ##
=======================================
- Coverage 80.8% 80.7% -0.1%
=======================================
Files 111 111
Lines 9257 9301 +44
=======================================
+ Hits 7480 7515 +35
- Misses 1777 1786 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a6a9f1f to
1390a7e
Compare
jerbly
left a comment
There was a problem hiding this comment.
Looks good... just needs a new error variant/s to clarify write errors.
|
Sorry @lmolkova - I just noticed that usage.md may have been hand edited. I've done a bad job explaining that this is now generated. Update the /// doc comments in the code and then run: |
| /// This is produced by `weaver registry package` and describes the contents of | ||
| /// a self-contained registry artifact, including its resolved schema location. | ||
| #[derive(Serialize, Deserialize, Debug, Clone, JsonSchema)] | ||
| pub struct PublicationRegistryManifest { |
There was a problem hiding this comment.
let's try putting it into resolved schema crate
There was a problem hiding this comment.
I tried, essentially it means have duplicate structure, one unnamed here and another one named there and some copy logic. but in practice it does not seem to bring any benefit.
I wonder if separation between semconv and resolved crates is even necessary? you can't use semconv without resolving them.
src/registry/package.rs
Outdated
| })? | ||
| .clone(); | ||
|
|
||
| let loaded = weaver.load_definitions(main_registry_repo, &mut diag_msgs)?; |
There was a problem hiding this comment.
I'm so happy this API is working out for us, when I see it used in commands like this.
Any friction / oddities in weaver here? Should we perhaps have it also create RegsitryRepo?
There was a problem hiding this comment.
yep, it's super-easy to use, no friction at all!
I think we should do a big cleanup when we remove legacy and use this chance to polish this - repo seems like an impl detail that would be cool to hide.
I don't think it's a big deal unless we want to ship libraries and need good public api
jsuereth
left a comment
There was a problem hiding this comment.
This looks great to me. Pre-approving but I can re-review once the fixes for registry types are in.
| let expected_errors: String = std::fs::read_to_string(&expected_errors_file) | ||
| .expect("Failed to read expected errors file"); | ||
| let observed_errors = serde_json::to_string(&schema).unwrap(); | ||
| let observed_errors = serde_json::to_string(&schema.err()).unwrap(); |
There was a problem hiding this comment.
schema does not need to be serializeable, it was the only place that required it to be, but we don't care about schema itself, only about errors.
| "title": "DefinitionRegistryManifest", | ||
| "description": "Represents the definition manifest for a semantic convention registry.\n\nThis is used when developing a registry before it is published.\nSee [`PublicationRegistryManifest`] for the stricter publication form produced\nby `weaver registry package`.", | ||
| "type": "object", | ||
| "properties": { |
There was a problem hiding this comment.
no file format here. not sure we need one for definition, let's add it when necessary.
Fixes #1244