Skip to content

Conversation

jpkrohling
Copy link
Member

@jpkrohling jpkrohling commented Sep 18, 2020

Description: Added a new internal/auth package with the interface that should be implemented by authenticators. Split from #1728.

This PR is based on top of #1807.

Link to tracking Issue: Part of the solution for #1424.

Testing: unit tests.

Documentation: none.

Signed-off-by: Juraci Paixão Kröhling [email protected]

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
@codecov
Copy link

codecov bot commented Sep 18, 2020

Codecov Report

Merging #1808 into master will decrease coverage by 0.80%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1808      +/-   ##
==========================================
- Coverage   91.95%   91.14%   -0.81%     
==========================================
  Files         263      265       +2     
  Lines       18821    16131    -2690     
==========================================
- Hits        17307    14703    -2604     
+ Misses       1083     1001      -82     
+ Partials      431      427       -4     
Impacted Files Coverage Δ
internal/auth/authenticator.go 100.00% <100.00%> (ø)
internal/auth/context.go 100.00% <100.00%> (ø)
service/internal/templates.go 30.43% <0.00%> (-21.09%) ⬇️
processor/cloningfanoutconnector.go 57.57% <0.00%> (-9.10%) ⬇️
cmd/mdatagen/metricdata.go 71.42% <0.00%> (-8.29%) ⬇️
exporter/kafkaexporter/otlp_marshaller.go 71.42% <0.00%> (-6.35%) ⬇️
...rnal/scraper/processesscraper/processes_scraper.go 80.00% <0.00%> (-5.72%) ⬇️
extension/pprofextension/pprofextension.go 57.14% <0.00%> (-5.36%) ⬇️
service/builder/extensions_builder.go 80.76% <0.00%> (-4.53%) ⬇️
...eceiver/internal/scraper/processscraper/factory.go 55.55% <0.00%> (-4.45%) ⬇️
... and 249 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 639b9a8...2214d1e. Read the comment docs.

@jpkrohling jpkrohling force-pushed the jpkrohling/authentication-processor-part-2 branch from f2c672e to 2214d1e Compare September 18, 2020 08:58
// See the License for the specific language governing permissions and
// limitations under the License.

package auth
Copy link
Member

Choose a reason for hiding this comment

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

Can this file be in config/configauth/internal/?

Copy link
Member Author

Choose a reason for hiding this comment

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

Only the interface, or the whole package? In any case, I would prefer to keep the config package clean from the implementation, as seems to be the case today.

Copy link
Member

Choose a reason for hiding this comment

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

that's why I am suggesting an internal package inside the configauth which will not be exported. But this way we have localization, the current internal/auth is only used by configauth

Copy link
Member Author

Choose a reason for hiding this comment

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

Except that the gRPC and HTTP receivers will use a default implementation for the interceptors/handlers, also located in this package.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's move forward with this (I don't believe that we cannot just have it in the internal here). But let's see.

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'll collect the non-blocking comments in an issue, so that I can solve them in a follow-up PR. To be honest, my biggest contention right now in changing this PR is in rebasing the other two PRs again.

Copy link
Member Author

@jpkrohling jpkrohling Sep 24, 2020

Choose a reason for hiding this comment

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

Tracker: #1824

@tigrannajaryan
Copy link
Member

@bogdandrutu I am assigning this series to you, assuming you want to review it.

@bogdandrutu bogdandrutu merged commit 430c002 into open-telemetry:master Sep 24, 2020
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
The initialization options of the exporter are not used after the
Exporter is created. Stop saving them in a field.
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
* Update java agent to v1.14.0

* Update smart agent to v5.22.0

* Update signalfxexporter to 05f3ece

* Update changelog
Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024
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