Skip to content

[extensionauth] Split extensionauth.Client by protocol type#12574

Merged
mx-psi merged 10 commits into
open-telemetry:mainfrom
mx-psi:mx-psi/extensionauth-refactor
Mar 12, 2025
Merged

[extensionauth] Split extensionauth.Client by protocol type#12574
mx-psi merged 10 commits into
open-telemetry:mainfrom
mx-psi:mx-psi/extensionauth-refactor

Conversation

@mx-psi
Copy link
Copy Markdown
Member

@mx-psi mx-psi commented Mar 6, 2025

Description

An attempt at splitting extensionauth.Client by protocol type. open-telemetry/opentelemetry-collector-contrib/pull/38451 removes usages of the NewClient and NewServer constructor

@mx-psi mx-psi changed the title mx psi/extensionauth refactor [PoC] Split extensionauth.Client by protocol type Mar 6, 2025
Comment thread extension/extensionauth/client.go Outdated
Comment thread extension/extensionauth/client.go Outdated
@mx-psi mx-psi force-pushed the mx-psi/extensionauth-refactor branch from 5427cb5 to 9423499 Compare March 7, 2025 13:16
@mx-psi mx-psi force-pushed the mx-psi/extensionauth-refactor branch from 9423499 to 71bb798 Compare March 7, 2025 13:17
@mx-psi mx-psi changed the title [PoC] Split extensionauth.Client by protocol type [extensionauth] Split extensionauth.Client by protocol type Mar 7, 2025
@mx-psi mx-psi added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Mar 7, 2025
@mx-psi mx-psi marked this pull request as ready for review March 7, 2025 13:19
@mx-psi mx-psi requested a review from a team as a code owner March 7, 2025 13:19
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 7, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 20 lines in your changes missing coverage. Please review.

Project coverage is 92.06%. Comparing base (b8830dd) to head (40ee155).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
extension/extensionauth/client.go 0.00% 10 Missing ⚠️
config/configauth/configauth.go 57.14% 5 Missing and 1 partial ⚠️
...sion/extensionauth/extensionauthtest/nop_server.go 0.00% 4 Missing ⚠️

❌ Your patch status has failed because the patch coverage (50.00%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12574      +/-   ##
==========================================
- Coverage   92.16%   92.06%   -0.10%     
==========================================
  Files         471      473       +2     
  Lines       25561    25590      +29     
==========================================
+ Hits        23558    23560       +2     
- Misses       1591     1617      +26     
- Partials      412      413       +1     

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread extension/extensionauth/extensionauthtest/error_client.go Outdated
Comment thread extension/extensionauth/extensionauthtest/error_client.go Outdated
@mx-psi mx-psi requested a review from bogdandrutu March 7, 2025 16:36
Copy link
Copy Markdown
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

@bogdandrutu bogdandrutu added the release:blocker The issue must be resolved before cutting the next release label Mar 10, 2025
Copy link
Copy Markdown
Contributor

@jade-guiton-dd jade-guiton-dd left a comment

Choose a reason for hiding this comment

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

LGTM, just a few nitpicks on the doc comments

Comment thread extension/extensionauth/client.go Outdated
Comment thread extension/extensionauth/client.go Outdated
mx-psi and others added 2 commits March 11, 2025 15:57
Co-authored-by: Jade Guiton <jade.guiton@datadoghq.com>
@mx-psi
Copy link
Copy Markdown
Member Author

mx-psi commented Mar 11, 2025

I addressed all comments, however I do not want to merge this until the discussion over at #12601 is finalized

@mx-psi
Copy link
Copy Markdown
Member Author

mx-psi commented Mar 12, 2025

Given #12601 (comment) I am going to merge this

@mx-psi mx-psi added this pull request to the merge queue Mar 12, 2025
Merged via the queue into open-telemetry:main with commit 1012122 Mar 12, 2025
@mx-psi mx-psi deleted the mx-psi/extensionauth-refactor branch March 12, 2025 12:18
mx-psi added a commit to open-telemetry/opentelemetry-collector-contrib that referenced this pull request Mar 12, 2025
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Prepares for addressing changes from
open-telemetry/opentelemetry-collector#12574
mx-psi added a commit to open-telemetry/opentelemetry-collector-contrib that referenced this pull request Mar 13, 2025
…mentions (#38601)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

<!-- Issue number (e.g. #1234) or full URL to issue, if applicable. -->

Follows #38451 and open-telemetry/opentelemetry-collector/pull/12574.

Removes explicit mentions to `extensionauth.Client` and replaces them by
`extensionauth.HTTPClient` and `extensionauth.GRPCClient`
github-merge-queue Bot pushed a commit that referenced this pull request Mar 19, 2025
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Removes deprecated symbols from `extensionauth` and `configauth` modules

<!-- Issue number if applicable -->
#### Link to tracking issue
Follows #12574
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

exporter/otlp release:blocker The issue must be resolved before cutting the next release Skip Changelog PRs that do not require a CHANGELOG.md entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants