-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
KAFKA-6562: (follow-up) Publish "jackson-databind" lib as provided scope dependency in clients maven artifact #5110
Conversation
997e806
to
f4ce46e
Compare
@ijuma @rajinisivaram Please take a look when you get a chance. sample generated pom file: |
compile libs.jacksonDatabind // for SASL/OAUTHBEARER bearer token parsing | ||
compileOnly libs.jacksonDatabind // for SASL/OAUTHBEARER bearer token parsing | ||
|
||
jacksonDatabindConfig libs.jacksonDatabind // to publish as provided scope dependency. |
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.
What happens if we don't include this?
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.
We will not get below provided scope dependency in clients maven pom file. Like regular maven based projects, it is good to publish provided scope libraries. Otherwise users will not get to know all the required libs (users may not always read the docs).
<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-databind</artifactId>
<version>2.9.5</version>
<scope>provided</scope>
</dependency>
f4ce46e
to
4ab3af2
Compare
retest this please |
build.gradle
Outdated
jacksonDatabindConfig | ||
} | ||
|
||
conf2ScopeMappings.addMapping(1000, configurations.jacksonDatabindConfig, "provided") |
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.
What does 1000
mean here? Probably worth adding a comment.
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.
Its a priority number used for comparison with the priority of other scopes. If multiple configurations are mapped with different scopes, then high priority scope will be select ed.
It on our case we have only one configuration, So its not a issue. I just gave 1000 (highest priority) for the provided scope.
Added a comment.
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 apart from one minor comment. @rajinisivaram, does this look good to you too?
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.
@ijuma Can we merge this? |
docs/security.html
Outdated
@@ -712,6 +712,9 @@ <h3><a id="security_sasl" href="#security_sasl">7.3 Authentication using SASL</a | |||
<pre> | |||
security.protocol=SASL_SSL (or SASL_PLAINTEXT if non-production) | |||
sasl.mechanism=OAUTHBEARER</pre></li> | |||
<li>The default implementation of SASL/OAUTHBEARER depends on com.fasterxml.jackson.core:jackson-databind library. | |||
This is added as provided scope dependency in Kafka clients maven artifact. Users must provide jackson-databind library | |||
to Kafka client runtime.</li> |
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 didn't notice this the first time, but this wording is a bit unclear. Maybe something like:
"The default implementation of SASL/OAUTHBEARER depends on the jackson-databind library. Since it's an optional dependency, users have to configure it as a dependency via their build tool. The maven coordinates are..."
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.
Updated docs. Since correct version will be available in pom file and it can change from release to release, I have not included maven links. Pls take a look.
…ope dependency in clients artifact
0eb44e4
to
105129c
Compare
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
Use `provided` scope in Maven. Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com>, Ismael Juma <ismael@juma.me.uk>
…che#5110) Use `provided` scope in Maven. Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com>, Ismael Juma <ismael@juma.me.uk>
https://docs.gradle.org/current/javadoc/org/gradle/api/artifacts/maven/Conf2ScopeMappingContainer.html
Committer Checklist (excluded from commit message)