Skip to content
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

CC-14690 : Define com.fasterxml.jackson.dataformat:jackson-dataformat-cbor. #334

Merged
merged 2 commits into from May 11, 2021

Conversation

xjin-Confluent
Copy link
Member

@xjin-Confluent xjin-Confluent commented May 10, 2021

This is to define the com.fasterxml.jackson.dataformat:jackson-dataformat-cbor version as a property in common so that all downstream consumers can use the property. The purpose is to unify the non-vulnerable version of shared libraries in common. This provides convenience when we need to upgrade this in the future.

We could define this in the dependencyManagement in the specific repo, but that does no scale because this could be used in multiple places. For now, this will be used confluentinc/kafka-connect-storage-cloud#415

Copy link
Contributor

@ewencp ewencp left a comment

Choose a reason for hiding this comment

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

Can you include a ticket ID at the front of the PR title and an explanation of the motivation? By itself this doesn't really do anything, and if we need to explicitly override a specific jar, I would expect there to be an addition to the dependencyManagement section.

@xjin-Confluent xjin-Confluent changed the title Define com.fasterxml.jackson.dataformat:jackson-dataformat-cbor. CC-14690 : Define com.fasterxml.jackson.dataformat:jackson-dataformat-cbor. May 10, 2021
@xjin-Confluent
Copy link
Member Author

Can you include a ticket ID at the front of the PR title and an explanation of the motivation? By itself this doesn't really do anything, and if we need to explicitly override a specific jar, I would expect there to be an addition to the dependencyManagement section.

Yep. Included the ticket id and also add context in the description.

@ewencp
Copy link
Contributor

ewencp commented May 10, 2021

We could define this in the dependencyManagement in the specific repo, but that does no scale because this could be used in multiple places.

You would put it in dependencyManagement here, and then downstreams would not specify a version, they should just use the one here by default if they specify just the group and artifactId of the dependency. That's one of the values of defining things in dependencyManagement in this repo -- it makes it easy to do platform-wide upgrades of dependencies with a single change here (assuming no repos have overridden the values, which is something we keep an eye out for when reviewing PRs that add dependencies to downstream repos).

@xjin-Confluent
Copy link
Member Author

We could define this in the dependencyManagement in the specific repo, but that does no scale because this could be used in multiple places.

You would put it in dependencyManagement here, and then downstreams would not specify a version, they should just use the one here by default if they specify just the group and artifactId of the dependency. That's one of the values of defining things in dependencyManagement in this repo -- it makes it easy to do platform-wide upgrades of dependencies with a single change here (assuming no repos have overridden the values, which is something we keep an eye out for when reviewing PRs that add dependencies to downstream repos).

Added dependecy and also https://github.com/confluentinc/kafka-connect-storage-cloud/pull/415/files

Copy link
Contributor

@ewencp ewencp left a comment

Choose a reason for hiding this comment

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

LGTM as long as we've a) verified this against the other PR (and e.g. that there aren't compatibility issues between this version and the other jackson-bom versions) and b) this is the oldest branch we need this on.

@xjin-Confluent xjin-Confluent merged commit d83928e into 5.4.x May 11, 2021
xjin-Confluent added a commit that referenced this pull request May 26, 2021
…-cbor. (#334)

* Define com.fasterxml.jackson.dataformat:jackson-dataformat-cbor version property

* Fix comment
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.

None yet

2 participants