Skip to content

Conversation

@trask
Copy link
Member

@trask trask commented Oct 26, 2024

No description provided.

@codecov
Copy link

codecov bot commented Oct 26, 2024

Codecov Report

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

Project coverage is 90.48%. Comparing base (2e8bf7f) to head (67e05c0).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...nfigure/AutoConfiguredOpenTelemetrySdkBuilder.java 20.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6824      +/-   ##
============================================
- Coverage     90.49%   90.48%   -0.02%     
  Complexity     6598     6598              
============================================
  Files           731      731              
  Lines         19735    19736       +1     
  Branches       1938     1938              
============================================
- Hits          17860    17858       -2     
- Misses         1283     1285       +2     
- Partials        592      593       +1     

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

Comment on lines -71 to +73
-Porg.gradle.java.installations.paths=${{ steps.setup-java-test.outputs.path }},${{ steps.setup-java.outputs.path }}
"-Porg.gradle.java.installations.paths=${{ steps.setup-java-test.outputs.path }}"
"-Porg.gradle.java.installations.auto-download=false"
Copy link
Member Author

Choose a reason for hiding this comment

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

I think the double quotes ended up being the only thing that was needed here, but the other changes seem good and align with the opentelemetry-java-instrumentation project (where, btw, we only run smoke tests against windows but not regular integration or unit tests, so haven't run into this double quote issue)

@trask trask marked this pull request as ready for review October 26, 2024 22:44
@trask trask requested a review from a team as a code owner October 26, 2024 22:44
- name: Check for diff
# The jApiCmp diff compares current to latest, which isn't appropriate for release branches, or for bot-generated PRs
if: ${{ !startsWith(github.ref_name, 'release/') && !startsWith(github.base_ref, 'release/') && (github.actor != 'opentelemetrybot') }}
if: ${{ matrix.os != 'windows-latest' && !startsWith(github.ref_name, 'release/') && !startsWith(github.base_ref, 'release/') && (github.actor != 'opentelemetrybot') }}
Copy link
Member

Choose a reason for hiding this comment

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

#nit: maybe update the comment to specify why its not required on windows?

Copy link
Member Author

Choose a reason for hiding this comment

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

added comment

throw new ConfigurationException("Configuration file not found", e);
}
try {
try (FileInputStream fis = new FileInputStream(configurationFile)) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch here to close this. Is is really the case that IOException is only thrown when an error occurs closing the file?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I was kinda surprised by that too, I'll add a comment in that catch block

@jack-berg jack-berg merged commit ff4fe97 into open-telemetry:main Nov 1, 2024
@trask trask deleted the windows branch October 20, 2025 17:29
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