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

GEODE-7294: Update dependencies for v1.11 #4104

Closed
wants to merge 1 commit into from

Conversation

metatype
Copy link
Contributor

@metatype metatype commented Oct 1, 2019

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

  • Has your PR been rebased against the latest commit within the target branch (typically develop)?

  • Is your initial contribution a single, squashed commit?

  • Does gradlew build run cleanly?

  • Have you written or updated unit tests to verify your changes?

  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

Note:

Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.

@metatype metatype force-pushed the develop branch 2 times, most recently from 4907643 to cc6663f Compare October 2, 2019 23:44
@metatype metatype force-pushed the develop branch 3 times, most recently from c6b66e9 to 40ea7d6 Compare October 14, 2019 23:46
@metatype metatype changed the title Updating some dependencies, just testing... GEODE-7294: Update dependencies for v1.11 Oct 15, 2019
@metatype metatype requested review from a user, dickcav and jdeppe-pivotal October 15, 2019 00:03
jdeppe-pivotal
jdeppe-pivotal previously approved these changes Oct 15, 2019
@jdeppe-pivotal
Copy link
Contributor

@metatype LMK if you'd like some help fixing the failures.

dickcav
dickcav previously approved these changes Oct 15, 2019
Copy link
Contributor

@dickcav dickcav left a comment

Choose a reason for hiding this comment

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

I'm assuming we can get the check tests to pass? Should this be pushed through other down stream testing too for further verification before merging it?

api(group: 'com.zaxxer', name: 'HikariCP', version: '3.2.0')
api(group: 'commons-beanutils', name: 'commons-beanutils', version: '1.9.3')
api(group: 'com.sun.istack', name: 'istack-commons-runtime', version: '3.0.9')
api(group: 'com.tngtech.archunit', name:'archunit-junit4', version: '0.11.0')
Copy link

Choose a reason for hiding this comment

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

We're upgrading major versions of dependencies here, in the context of a minor of Geode. If we emit object from the packages as part of our API, then we might be breaking downstream clients.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these should be safe to upgrade.

@@ -24,7 +24,7 @@ dependencies {
compile(platform(project(':boms:geode-all-bom')))
gradleLint.ignore {
// See GEODE-6128 -- ignore xml-apis in linter to avoid changes with every run.
upgradeTestCompile('xml-apis:xml-apis:1.4.01')
upgradeTestCompile('xml-apis:xml-apis:2.0.2')
Copy link

Choose a reason for hiding this comment

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

Lets take this opportunity to move this version into our dependency manager, instead of being the sole hard-coded number.

testCompile(group: 'net.sourceforge.pmd', name: 'pmd-test', version: '6.18.0')
Copy link

Choose a reason for hiding this comment

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

Lets take this opportunity to move this version into our dependency manager, instead of being the sole hard-coded number.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

GitHub is making me repeat my change request. All versions in *.gradle files should be put into DependencyConstraints.groovy wherever possible.

@metatype metatype force-pushed the develop branch 2 times, most recently from e31c88c to 2eca393 Compare October 22, 2019 17:09
@metatype
Copy link
Contributor Author

GitHub is making me repeat my change request. All versions in *.gradle files should be put into DependencyConstraints.groovy wherever possible.

I agree in principle, but I'd like to address that cleanup in a separate PR.

@metatype
Copy link
Contributor Author

I'm assuming we can get the check tests to pass? Should this be pushed through other down stream testing too for further verification before merging it?

I'm working on a timeout failure in AvailableConnectionManagerConcurrentTest. Currently trying to find the maximal set of upgrades that will pass tests.

ghost
ghost previously approved these changes Oct 28, 2019
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I agree to this change, and we'll get consistency with the xml-apis dependency in a different commit.

Update as many dependencies as we can for v1.11. Since
so many updates are included, please review the change
set to see them all. Not *all* dependencies have been
advanced; only the ones that seemed safest to do easily.
@metatype
Copy link
Contributor Author

Closing this PR, superseded by newer work.

@metatype metatype closed this Dec 26, 2019
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