Repository navigation
Feature/1004 update javalin drop tomcat - #1985
MikeNeilson wants to merge 92 commits into
Conversation
|
NOTE: at best I will have the annotations updated today. Getting the API running again in it's standalone mode will likely take until the end of the week if not longer. |
|
For the record. I have considered keeping with tomcat and just using like embedded tomcat for the packaging, but Javalin was made for Jetty so since that would be a fair amount of work anyway, better to use a tool that was actually built for this given style of packaging an app. |
|
FYI, I am aware this PR is rather huge. The OpenAPI changes basically have to be all or nothing. I think we can all agree on that. It also requires the migration to Jakarta EE (javalin update) which would may or may not (java.security.Principal so probably not) affect the CwmsAaaIdentityProvider. So fair amount of changes to Tomcat anyway. So is this likely to be a pain... yes, but I'm arguing it's worth it. I could pivot back and do the move to standalone Javalin (e.g. Jetty whatever version that was) first, but then without updating javalin frankly it wouldn't be in a releasable state (security concerns, that's a really old Jetty) So. for now I'm going to barrel through. I'm open to addressing concerns and pivoting if they're strong enough. We do have a lot of plans. and the bulk are passing. What's failing is most likely various validation I've intentionally disabled due to the location of initialization changing. |
e041252 to
0263cc0
Compare
Documentation build overview
13 files changed ·
|
|
I'm still working through dealing with the lock icons. the behavior as a whole has change. I may need to be drastic and take the given out put to json, and put it through the Swagger OpenAPI object, but i'm hoping to do it all within the javalin openapi annotations. I think most of issue is assumptions we previously made that are no longer required that may be getting in the way. The primary work of the Javalin update and removal of Tomcat are finished and should be reviewed. I think I got all of the paths in correctly, but it's definitely possible that some have been missed that also weren't detected by the various validations, new and old. NOTE: This does break /CWMSLogin. That is intentional and will be dealt with in a follow up. Once merged CDA is a standalone app like the CWMS Server apps and thus won't be getting randomly deployed to a T7 any time soon. |
33e7c77 to
c7d3788
Compare
Co-authored-by: Ryan Ripken <89810919+rma-rripken@users.noreply.github.com>
bec683a to
62a5c9e
Compare
|
Okay, I think that's it. Or at the very least, I think that's all we're going to find without just moving forward. Too many staged PR that need different imports and certain structural elements to let this linger too much longer even if it might break a few things. |
|
@oskarhurst @rma-rripken @rma-psmorris NOTE: I'm aware it's a bit, abrupt, but I don't want this to lag. Should I here nothing by end of day Friday I'm going to merge and accept the consequences. |
| @JsonProperty("datatype") | ||
| @OpenApiName("datatype") |
There was a problem hiding this comment.
weird that we need JsonProperty and OpenApiName
There was a problem hiding this comment.
Obnoxious is the better word.
We use a specific FasterXML library library to do the mapping, it uses JsonProperty.
The updated javalin-openapi does it's own thing, thus the addition of the OpenApiName annotation.
@MikeNeilson -- could you delay this for a week? I'm setup to go through this review and would like to be able to do so. I'm off tomorrow so would not be able to complete a review before end of day. I will certainly be able to do my review next week. |
I would prefer not to; however, there's already going to be follow up, there's a few known things, and I changed 500+ files - there's no way we caught everything. So there will still be value in that review. The burden of updates to this PR is getting high. And there's about 4-5 more that need to start getting merged in. I want to just get it over with, the initial hurdle anyway. So no, but this PR being closed doesn't me you shouldn't review anyway so things can get fixed. |
| tags = {TAG} | ||
| methods = HttpMethod.POST, | ||
| tags = {TAG}, | ||
| path = "/v2/blob/" |
There was a problem hiding this comment.
Doesn't this class get registered as /v2/blobs/ ?
There was a problem hiding this comment.
wait, yeah, probably. Good catch, I actually meant to go back and check that. Will do so in the morning. (currently having network issues.)
Summary
Move to latest javalin (7.2.3)
Remove Use of Tomcat, no Tomcat 11+ in our local environment so will just use Javalin's default jetty and package it up.
NOTE: Due to Javalin updating the annotation plugin (this happened in javalin 5) this is going to be a large change as it touches every file and will need to alter how the OpenAPI static test works.
TODO:
Related Issue
Validation
Existing integration tests
Checklist