-
Notifications
You must be signed in to change notification settings - Fork 16
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
Update Spring Boot to 2.7.6 #47
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, I did make some minor changes to the docker base image to be in line with cBioPortal/cbioportal#9890 (hope that's ok)
We don't really have good testing set up for session service functionality in isolation (just running all the cbioportal e2e tests against it I guess)
Did y'all test this in any way @pvannierop or @oplantalech ?
@@ -0,0 +1 @@ | |||
spring.mvc.pathmatch.matching-strategy=ant-path-matcher |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pvannierop @oplantalech actually one comment about this. Is there a way we can pass this in another way. Reason I'm asking is that I would ideally prefer installers having to update their existing application.properties
files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@inodb
How this prop is passed to the application indeed is the responsibility of the installer. Or did not mean I would ideally prefer installers not having to update their existing application.properties files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry yes I meant not having to update their existing files. Especially like in this case where it really shouldn't be a user customizable property
Spring often has ways of configuring this through either properties or having it hardcoded. I prefer the latter if possible. Multiple property files could work too otherwise by putting all "don't touch this" configurable spring stuff in one file
Hi Ino,
Yes, I tested it and it works.
Bye, Pim
…On Fri, Dec 23, 2022 at 3:20 PM Ino de Bruijn ***@***.***> wrote:
***@***.**** approved this pull request.
This looks good to me, I did make some minor changes to the docker base
image to be in line with cBioPortal/cbioportal#9890
<cBioPortal/cbioportal#9890> (hope that's ok)
We don't really have good testing set up for session service functionality
Did y'all test this in any way @pvannierop <https://github.com/pvannierop>
or @oplantalech <https://github.com/oplantalech> ?
—
Reply to this email directly, view it on GitHub
<#47 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFWDHL6L2OUJHAE2FUUSF3WOWYK7ANCNFSM6AAAAAATGQI7H4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Pim van Nierop
Software Engineer / cBioPortal specialist
E ***@***.***
T +31(0)30 700 9713
M +31(0)6 29464525
W thehyve.nl <https://www.thehyve.nl/>
<https://www.linkedin.com/company/thehyve/> <http://twitter.com/thehyvenl>
<https://github.com/thehyve>
|
PR feedback Ino
f6646ad
to
aad9666
Compare
Client pointed out a security issue with the current version of Spring. This PR will upgrade Spring Boot to 2.7.6 and use JRE 11.