-
Notifications
You must be signed in to change notification settings - Fork 45
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
Add oauth2 configuration (2/2) #172
Conversation
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.
Some comments on the main implementation. Will look into the tests a bit later.
Thanks @aprodhomme-dkt !
src/main/java/io/aiven/kafka/connect/http/sender/request/OAuth2Form.java
Outdated
Show resolved
Hide resolved
src/main/java/io/aiven/kafka/connect/http/sender/request/OAuth2Form.java
Outdated
Show resolved
Hide resolved
src/main/java/io/aiven/kafka/connect/http/sender/request/OAuth2Form.java
Outdated
Show resolved
Hide resolved
src/main/java/io/aiven/kafka/connect/http/config/HttpSinkConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/io/aiven/kafka/connect/http/config/HttpSinkConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/io/aiven/kafka/connect/http/sender/request/OAuth2Form.java
Outdated
Show resolved
Hide resolved
src/main/java/io/aiven/kafka/connect/http/sender/request/OAuth2Form.java
Outdated
Show resolved
Hide resolved
src/main/java/io/aiven/kafka/connect/http/sender/request/OAuth2Form.java
Outdated
Show resolved
Hide resolved
src/main/java/io/aiven/kafka/connect/http/sender/request/OAuth2Form.java
Outdated
Show resolved
Hide resolved
src/main/java/io/aiven/kafka/connect/http/sender/request/OAuth2Form.java
Outdated
Show resolved
Hide resolved
@aprodhomme-dkt could you rebase changes from main, so we can review commits only for this change. ps. forgot to squash commits from #171, so main history got messed up a bit. |
done. |
Commits seem a bit strange still. It should only have the last 5 included in your PR, but there are 21. |
indeed. I'll try to make it clearer. I might have to push force. |
I think you're good to go now @jeqo ;) |
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.
@aprodhomme-dkt Looking good! just a few minor comments and validation improvement suggestions.
src/main/java/io/aiven/kafka/connect/http/config/HttpSinkConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/io/aiven/kafka/connect/http/sender/request/OAuth2AccessTokenRequestForm.java
Outdated
Show resolved
Hide resolved
src/main/java/io/aiven/kafka/connect/http/sender/OAuth2AccessTokenHttpSender.java
Outdated
Show resolved
Hide resolved
src/main/java/io/aiven/kafka/connect/http/config/HttpSinkConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/io/aiven/kafka/connect/http/config/HttpSinkConfig.java
Outdated
Show resolved
Hide resolved
@aprodhomme-dkt sorry for the delay. Hope to look into this later this week. Thanks. |
@aprodhomme-dkt thanks for looking into the suggestions! I think there's only one missing and it should be good to go. |
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.
LGTM, thanks @aprodhomme-dkt for your contribution!
This PR comes after having reviewed and merged the PR #171
It provides an optional configuration to use with API publishers that use OAUTH2 authentication but with their own form fields.
For example requesting an access token with the following form (eg: https://api.didomi.io/docs/#/sessions/post_sessions):
We would have the following configuration: