Skip to content

GEODE-6557: Moving geode-web classes to geode-web#3351

Merged
upthewaterspout merged 3 commits intoapache:developfrom
upthewaterspout:feature/move-admin-web-GEODE-6557
Mar 27, 2019
Merged

GEODE-6557: Moving geode-web classes to geode-web#3351
upthewaterspout merged 3 commits intoapache:developfrom
upthewaterspout:feature/move-admin-web-GEODE-6557

Conversation

@upthewaterspout
Copy link
Copy Markdown
Contributor

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.

@upthewaterspout upthewaterspout force-pushed the feature/move-admin-web-GEODE-6557 branch from 1edaeaa to 3e209a7 Compare March 22, 2019 23:21
Moving the classes in geode-core that really should be in geode-web.
This removes optional dependencies from geode-core and puts these
classes where their tests are.

As part of this change, SerializableObjectHttpMessageConverter is now
duplicated in geode-web as ServerSerializableObjectHttpMessageConverter.
This class was previously put in both the geode-web.war file and the
gfsh-dependencies by creating a separate jar with all of the geode-web
classes as part of the geode-core build.

This class is used by both gfsh and geode-web. But geode-web needs its
own copy so that the class is loaded by the geode-web war classloader in
order to ensure the class is linked with the spring-web classes coming
from the geode-web classloader.
@upthewaterspout upthewaterspout force-pushed the feature/move-admin-web-GEODE-6557 branch from 3e209a7 to 658d711 Compare March 25, 2019 18:21
Copy link
Copy Markdown
Contributor

@jdeppe-pivotal jdeppe-pivotal left a comment

Choose a reason for hiding this comment

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

LGTM - I tried (not very hard) removing ServerSerializableObjectHttpMessageConverter altogether but I think it's required for some gfsh query functionality.

Copy link
Copy Markdown

@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 love this refactor. Small change to prefer xyzTestImplementation over xyzTestCompile

Applying review comments and using implementation instead of compile for
test dependencies.
@upthewaterspout upthewaterspout requested a review from a user March 27, 2019 18:13
@upthewaterspout upthewaterspout merged commit afb3a4f into apache:develop Mar 27, 2019
@upthewaterspout upthewaterspout deleted the feature/move-admin-web-GEODE-6557 branch March 27, 2019 23:46
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.

2 participants