kblincoe / QualOpt_SE701

2 stars 15 forks source link

Spring boot upgrade to 2.0.0.RELEASE #108

Closed TheGuardianWolf closed 6 years ago

TheGuardianWolf commented 6 years ago

Description

These series of commits are changes to ensure compatiblity with JRE 9, fixing issue #49 . The issue suggested that one of the project's Java dependencies did not function on JRE 9.

Changes

A brief summary of the changes is written below.

Testing

There may be breaking changes that have not been discovered by me or @Nemeritz, requesting review from @everyone so that the pull won't mess up people's issue resolutions.

Please make a new integration branch and pull from spring-boot-upgrade in my repo, then perform a merge + test.

We have independently run tests on these already.

I'm suggesting waiting at least a week before merging this in, or until everyone is fine with these changes.

kblincoe commented 6 years ago

Please review @crat019 @dylHall @sidpartha1 @zaclochhead @umih874 @AprajitGandhi @Karim-C @Nemeritz @st970703 @pulkitkalra @victorlian @nateeo @jsoulsby @Xavier630 @JoeHorvath @oliver-allen @FMcIntosh @frederickfogerty @JGud007

nateeo commented 6 years ago

Also here's one way of doing an integration test on your open PRs (credit to @frederickfogerty):

make sure you've set upstream remote to this repo git remote add upstream https://github.com/kblincoe/QualOpt_SE701.git

make a copy branch off of your open feature branch that you want to test git checkout your branch git checkout -b integration-test

fetch this pr and merge it into your open feature branch git fetch upstream pull/108/head:pr/108 git merge pr/108

Should be able to test now

jsoulsby commented 6 years ago

I don't have any open PR's as of now, but will test my changes in #34 once they're complete.

TheGuardianWolf commented 6 years ago

@softeng-701 Seems like this has been checked off by most people, ready for merge now.

softeng-701 commented 6 years ago

@TheGuardianWolf Please squash your commits, thanks!

TheGuardianWolf commented 6 years ago

@softeng-701 I'm not sure what you mean by stashing the commits, my latest have been pushed to the branch and are things like removing the Dockerfile I forgot to get rid of and updating that README warning, so I want to get them in as well.

xavier630 commented 6 years ago

@TheGuardianWolf I suspect that you are meant to squash the commits.

TheGuardianWolf commented 6 years ago

In that case, do I squash the whole branch into a single commit? That's the only option I have for squashing via my GUI, unless the command line can squash non HEAD commits.

nateeo commented 6 years ago

@TheGuardianWolf yeah you should squash your whole branch into a single commit

TheGuardianWolf commented 6 years ago

This is as much as I can squash it, if I squash that final merge, it just asks me to do a merge again.

Which is really weird since there's a file that the merge did not change at all, and the readme should have updated inside the squashed commits.

You can see that the merged diff was 0.

nateeo commented 6 years ago

@TheGuardianWolf you could try cherry picking the actual commit

TheGuardianWolf commented 6 years ago

I have no idea why the README is causing a conflict, I just wrote a fresh commit after merging with upstream's version and selecting that to resolve.

softeng-701 commented 6 years ago

Sorry, it is actually 'squash'. Have all the team members reviewed? Ready to merge?

lilcham commented 6 years ago

@TheGuardianWolf @Nemeritz Hey, just wanted to let you guys know that due to the new functionality introduced to master, we've found a semantic error which is causing the build to fail.

Attached is a screenshot of the build failure.

screen shot 2018-03-26 at 1 52 41 pm

The problem is due to the removal of findOne() and the return type of findById() being an optional.

@KijongHan Since this is affecting your class, can we work with @TheGuardianWolf and @Nemeritz to try and resolve this?

Cheers

UPDATE @dylHall and I investigated and found a quick bandaid fix for JDK 9. The file causing the build failure was the DocumentResource.java file and the line causing the failure was around line 36. The code can be replaced with this:

Optional<Document> documentOptional = documentRepository.findById(id);
if(documentOptional.isPresent()) {
    return new ResponseEntity<>(HttpStatus.NOT_FOUND);
}
Document document = documentOptional.get();
softeng-701 commented 6 years ago

@TheGuardianWolf bump! please review the changes and squash!

TheGuardianWolf commented 6 years ago

@softeng-701 Doing the changes now, these should really be merged in soon, the longer it waits the more merge conflicts and build errors will result due to Spring Boot API changes rather than Java 9.

If this is merged in, people can start writing with the new API rather than continue on with JB 1.5, then I won't have to keep replacing the deprecated functions.

TheGuardianWolf commented 6 years ago

@softeng-701 I'm not able to resolve test errors on this branch due to some changes, I can't find which pull broke the liquibase configuration.

Schema-validation: wrong column type encountered in column [file_image] in table [document]; found [clob (Types#CLOB)], but expecting [varchar(255) (Types#VARCHAR)]

Also, how come I can't squash a merge and keep the merged changes? I've been unable to keep all my changes each time I squash.

KijongHan commented 6 years ago

@TheGuardianWolf Hi, that issue should be fixed with #131 pr

lilcham commented 6 years ago

@TheGuardianWolf There might be another break in the liquibase configuration. I keep getting the error:

Schema-validation: wrong column type encountered in column [incentive_type] in table [study]; found [clob (Types#CLOB)], but expecting [blob (Types#BLOB)]
TheGuardianWolf commented 6 years ago

Yeah that is also what I am getting, we should be running tests pre-merge rather than finding these after. I've been holding this pull back a week to do testing with others.

Edit: Not sure if these are stricter rules imposed by liquibase on an updated version or if master is having these same issues?

TheGuardianWolf commented 6 years ago

Thanks for the fix @KijongHan

TheGuardianWolf commented 6 years ago

@softeng-701 As a sidenote, the pom.xml file is also broken, there's two versions of the same declared dependency in there.

softeng-701 commented 6 years ago

@TheGuardianWolf Has it been fixed? This PR took too long guys, any suggestions to take it forward?

TheGuardianWolf commented 6 years ago

@softeng-701 each time a new (Java based) pull is merged, a new conflict appears here. This was ready to merge last week after I squashed it, but the new pull merges is causing me to have to retest everything as everyone is still using the old API.

I also had to have a longer than usual review process with everyone rather than just my team, as it affects all of QualOpt essentially.

Please stop merging other Java based pulls before this one, it makes this issue larger.

softeng-701 commented 6 years ago

@TheGuardianWolf Okay. Please complete it asap. and let me know. Please meet me today, so that we can see if the issue size can be updated.