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

[DS-3804] Jar dependency issues running DSpace 7x REST #1921

Merged
merged 7 commits into from
Jan 23, 2018

Conversation

terrywbrady
Copy link
Contributor

@terrywbrady terrywbrady commented Jan 17, 2018

This PR is a temporary fix to restore stability to the REST build process.

Recommended approach

The more comprehensive approach to the problem will be to upgrade the version of log4j that is in use. DSpace currently uses a version that is at end of life.

@terrywbrady terrywbrady added bug quick win Pull request is small in size & should be easy to review and/or merge labels Jan 17, 2018
@terrywbrady terrywbrady added this to the 7.0 milestone Jan 18, 2018
@abollini abollini added the interface: REST API v7+ REST API for v7 and later (dspace-server-webapp module) label Jan 18, 2018
@abollini abollini self-requested a review January 18, 2018 13:48
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

Looks good overall. One minor request to add in a comment about the temporary dependency exclusions. But, after that, I think this is good to merge

@@ -169,6 +169,20 @@
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-web</artifactId>
<version>${spring-boot.version}</version>
<exclusions>
Copy link
Member

@tdonohue tdonohue Jan 19, 2018

Choose a reason for hiding this comment

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

Could we add a simple comment here that says something like:

<!-- Temporary exclusions until DS-3135 is fixed. Once we are using log4j v2, we should update Spring Boot to utilize it per these instructions: https://docs.spring.io/spring-boot/docs/current/reference/html/howto-logging.html#howto-configure-log4j-for-logging -->

Copy link
Member

@abollini abollini left a comment

Choose a reason for hiding this comment

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

if the jackson exclusion is not required to have the OAI running properly I will prefer to skip it

@@ -159,6 +159,10 @@
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
</exclusion>
<exclusion>
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit worried about this exclusion as it is an artifact from an old, different layout, jackson version. The mapper class are now in the jackson-databinder artifact so if we really use this staff we could hit a classnotfound exception at runtime somewhere... BTW the jtwig-spring version in our depency is quite old the new one is: http://jtwig.org/

@tdonohue tdonohue merged commit 42894a5 into DSpace:master Jan 23, 2018
@tdonohue tdonohue modified the milestones: 7.0, 7.0preview Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug interface: REST API v7+ REST API for v7 and later (dspace-server-webapp module) quick win Pull request is small in size & should be easy to review and/or merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants