Skip to content
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 optional variant to requirement_level #199

Merged
merged 2 commits into from
Jun 6, 2024

Conversation

MadVikingGod
Copy link
Contributor

@MadVikingGod MadVikingGod commented Jun 5, 2024

This enables weaver to parse semantic convention v1.18.0

This will enable parsing of semantic conventions v1.13.0 - v1.18.0

I was looking to put a test in crates/weaver_resolver/data, but I don't know if that's appropriate. And, I don't fully grok the format of what I should put there to make sure that it is parsing appropriately.

I also don't know how to signify that this is a deprecated option and shouldn't be used in general cases.

Expands the supported versions found in #181

This enables weaver to parse semantic convention v1.18.0
Copy link

codecov bot commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 74.3%. Comparing base (9ab536f) to head (96e9766).

Current head 96e9766 differs from pull request most recent head 78ad26b

Please upload reports for the commit 78ad26b to get more accurate results.

Files Patch % Lines
crates/weaver_semconv_gen/src/gen.rs 0.0% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #199     +/-   ##
=======================================
- Coverage   74.4%   74.3%   -0.1%     
=======================================
  Files         44      44             
  Lines       2918    2921      +3     
=======================================
  Hits        2172    2172             
- Misses       746     749      +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@lquerel lquerel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lquerel
Copy link
Contributor

lquerel commented Jun 6, 2024

I also don't know how to signify that this is a deprecated option and shouldn't be used in general cases.

@MadVikingGod Ideally, I think we should generate a violation via the policy engine when the semconv registry is greater than or equal to v1.19.0. However, the version number is not in the YAML docs themselves but in the form of a tag. So, we'll need to find a way to expose it to the policies one way or another. We should create a GH issue for that.

@lquerel
Copy link
Contributor

lquerel commented Jun 6, 2024

Added the policy into this list -> #185 (comment)

@lquerel lquerel merged commit 24965a1 into open-telemetry:main Jun 6, 2024
21 checks passed
@MadVikingGod MadVikingGod deleted the mvg/add-optional branch June 6, 2024 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants