-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Authentication processor 2/4 - Add auth context and interface #1808
Authentication processor 2/4 - Add auth context and interface #1808
Conversation
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
f2c672e
to
2214d1e
Compare
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
package auth |
There was a problem hiding this comment.
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/
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracker: #1824
@bogdandrutu I am assigning this series to you, assuming you want to review it. |
The initialization options of the exporter are not used after the Exporter is created. Stop saving them in a field.
* Update java agent to v1.14.0 * Update smart agent to v5.22.0 * Update signalfxexporter to 05f3ece * Update changelog
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]