Move Existing ServerCertificateValidation Classes to Shared #1550
Conversation
anoopbabu29
had a problem deploying
to
external
GitHub Actions
Failure
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1550 +/- ##
==========================================
- Coverage 73.91% 73.77% -0.14%
==========================================
Files 267 256 -11
Lines 9615 9415 -200
==========================================
- Hits 7107 6946 -161
+ Misses 2508 2469 -39
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
|
||
| namespace OpenTelemetry.ResourceDetectors; | ||
|
|
||
| [EventSource(Name = "OpenTelemetry-ResourceDetectors-Http")] |
There was a problem hiding this comment.
OpenTelemetry-ResourceDetectors-Http -- is there a component with this name?
There was a problem hiding this comment.
No, I have created that as a separate name as a placeholder.
There was a problem hiding this comment.
We should not use shared name for the eventsources. It'll cause issues like open-telemetry/opentelemetry-dotnet#4516 (comment)
There was a problem hiding this comment.
See https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1550/files#r1468194982
Each component should own its own logging, with unique eventsource name, else troubleshooting will be difficult.
anoopbabu29
had a problem deploying
to
external
GitHub Actions
Failure
anoopbabu29
had a problem deploying
to
external
GitHub Actions
Failure
anoopbabu29
had a problem deploying
to
external
GitHub Actions
Failure
…r to an interface on the event source
anoopbabu29
had a problem deploying
to
external
GitHub Actions
Failure
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
vishweshbankwar
had a problem deploying
to
external
GitHub Actions
Failure
@srprash @atshaw43 @ppittle This PR intends to move
I think individual components that end up using the |
utpilla
added
the
comp:resources.aws
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
| { | ||
| public static HttpClientHandler? Create(string certificateFile) | ||
| public static HttpClientHandler? Create(string certificateFile, IServerCertificateValidationEventSource? log = null) |
There was a problem hiding this comment.
Logging wasn't really optional before; I'm thinking log should be a required parameter.
I don't want future maintainers to think that log is optional, unless there is a clear (and documented) usecase as to when it doesn't need to be provided.
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
As @anoopbabu29 is not really active, I'm ready to help on this PR (especially to unblock #1562/#1529). If it's ok for you, you can close this PR and I'll open a new one then. |
|
Closing, per #1550 (comment) |
Attempt to move the ServerCertificateValidtion Classes to the Shared folder. This use case is very generic and has potential to be used for multiple ResourceDetector Projects. It was also intended to be used in the ResourceDetectors.Container Project in #1529.
One reservation I have for this is the ideal location for the test cases, as they currently just exist in the ResourceDetectors.AWS.Tests Test project.