-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ability to parse Schema files according to OTEP 0152 #2267
Conversation
@MrAlias @Aneurysm9 can you please have a look and tell me if this goes in the right direction? |
4c60e21
to
2901a57
Compare
Codecov Report
@@ Coverage Diff @@
## main #2267 +/- ##
=====================================
Coverage 72.6% 72.6%
=====================================
Files 170 171 +1
Lines 11873 11918 +45
=====================================
+ Hits 8621 8660 +39
- Misses 3019 3023 +4
- Partials 233 235 +2
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I think this looks good. Making this it's own module sounds like a good idea and I like the clear layout of code.
How do plan to handle version increments? I see the tests fail for v2.*
and v1.1.*
, but not v1.0.1
. I'm guessing for the patch version we expect to be compatible, I can see a v2
living in it's own package, but I'm wondering what the plan is for v1.1.*
and the like.
Yes, v2 should live in its own package and patch numbers are ignored since they are compatible. For minor number I think we can force the user to declare the maximum they expect. It is currently implied to be equal to 0, but we can make the Parse function accept maximum minor version number it expects, e.g.: func Parse(schemaFile string, maxMinorVerNum int) (*ast.Schema, error) {
}
// in checkFileFormatField compare minor number with maxMinorVerNum. Usage looks a bit ugly, not sure if we can make it nicer: const maxMinorVer = 2;
ts, err := schema.Parse("myschema.yaml", maxMinorVer) // maximum version number we can handle is 1.2.x. Maybe we can change a bit with how we supply the max minor number to make the usage more natural. I can play with some possible approaches to see what I can come up with, but the idea is going to be the same: force the user to declare what maximum version they expect. |
I thought a bit more and I think we should just make all major and all minor versions separate packages. The initial version will be Using New versions of file format are going to be rare (maybe 1-2 minor versions per year, ever less major versions) so we are not going to have a proliferation of packages. |
2413b13
to
5107269
Compare
@MrAlias Refactored. PTAL. |
If we are happy with this direction, I will tidy up the draft for full review. |
ce49916
to
d6e5d84
Compare
Squashed and ready for review. |
d6e5d84
to
6a62176
Compare
Please add the new module to |
155a461
to
f82962e
Compare
Done. |
All OTEP fixes that this depended on are now merged. This is ready for final review and merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall 👍, nothing blocking.
The parser and parsed representation (AST) are placed in a separate Go module so that they are can be consumed independently without the need to bring the rest of the SDK. Ability to use the parsed representation for schema conversions can be added later.
043dedc
to
2d0f80c
Compare
@Aneurysm9 do you want to have another look or we can merge? |
@open-telemetry/go-approvers is there anything else to do on this? |
Looks good. Going to merge. |
The parser and parsed representation (AST) are placed in a separate
Go module so that they are can be consumed independently without
the need to bring the rest of the SDK.
Ability to use the parsed representation for schema conversions
can be added later.