-
Notifications
You must be signed in to change notification settings - Fork 601
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
Small rye open api #13589
Small rye open api #13589
Conversation
#build |
Please code review feature-related files, @OpenLiberty/delivery-approvers |
Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_pbCioOOvEeqELpaeZj9ckg Target locations of links might be accessible only to IBM employees. |
Your Open Liberty build results are ready for viewing.
|
The build msmiths-13589-20200821-1422 For help analyzing your personal build, go to https://cognitive.hursley.ibm.com/buildAnalysis.html?uuid=_pbCioOOvEeqELpaeZj9ckg |
I have checked through the output of the personal build and the failures are unrelated to my changes. |
...here.appserver.features/visibility/public/mpOpenAPI-2.0/io.openliberty.mpOpenAPI-2.0.feature
Outdated
Show resolved
Hide resolved
bea0a66
to
5a828e1
Compare
#build |
Please code review feature-related files, @OpenLiberty/delivery-approvers |
Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_7Yk_4OYYEeql06cgdrhuYw Target locations of links might be accessible only to IBM employees. |
The build msmiths-13589-20200824-1601 For help analyzing your personal build, go to https://cognitive.hursley.ibm.com/buildAnalysis.html?uuid=_7Yk_4OYYEeql06cgdrhuYw |
Your Open Liberty build results are ready for viewing.
|
d45b014
to
d8ab652
Compare
#build |
Please code review feature-related files, @OpenLiberty/delivery-approvers |
Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_9ruLUObVEeql06cgdrhuYw Target locations of links might be accessible only to IBM employees. |
...rver.features/visibility/private/io.openliberty.org.eclipse.microprofile.openapi-2.0.feature
Outdated
Show resolved
Hide resolved
Your Open Liberty build results are ready for viewing.
|
The build msmiths-13589-20200825-1436 For help analyzing your personal build, go to https://cognitive.hursley.ibm.com/buildAnalysis.html?uuid=_9ruLUObVEeql06cgdrhuYw |
#build |
Please code review feature-related files, @OpenLiberty/delivery-approvers |
Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_7t95YOd-Eeql06cgdrhuYw Target locations of links might be accessible only to IBM employees. |
L2 message review completed |
Your Open Liberty build results are ready for viewing.
|
The build msmiths-13589-20200826-1046 |
...rofile.openapi_fat/fat/src/com/ibm/ws/microprofile/openapi/fat/ApplicationProcessorTest.java
Outdated
Show resolved
Hide resolved
...om.ibm.ws.microprofile.openapi_fat/fat/src/com/ibm/ws/microprofile/openapi/fat/FATSuite.java
Outdated
Show resolved
Hide resolved
This is absolutely horrible to review because you have lots of reformatting changes mixed in with code changes. If you're going to reformat the whole file, please do it in a separate commit because otherwise your real changes get lost amongst the noise of the formatting changes. |
...here.appserver.features/visibility/public/mpOpenAPI-2.0/io.openliberty.mpOpenAPI-2.0.feature
Outdated
Show resolved
Hide resolved
...here.appserver.features/visibility/public/mpOpenAPI-2.0/io.openliberty.mpOpenAPI-2.0.feature
Outdated
Show resolved
Hide resolved
dev/com.ibm.ws.microprofile.openapi_fat/.settings/org.eclipse.jdt.ui.prefs
Show resolved
Hide resolved
...icroprofile.openapi_fat/fat/src/com/ibm/ws/microprofile/openapi/fat/UICustomizationTest.java
Outdated
Show resolved
Hide resolved
...api_fat/fat/src/com/ibm/ws/microprofile/openapi/validation/fat/OpenAPIValidationTestOne.java
Outdated
Show resolved
Hide resolved
...file.openapi.2.0.internal/src/io/openliberty/microprofile/openapi20/ApplicationListener.java
Outdated
Show resolved
Hide resolved
...ile.openapi.2.0.internal/src/io/openliberty/microprofile/openapi20/ApplicationProcessor.java
Outdated
Show resolved
Hide resolved
...ile.openapi.2.0.internal/src/io/openliberty/microprofile/openapi20/ApplicationProcessor.java
Show resolved
Hide resolved
...ile.openapi.2.0.internal/src/io/openliberty/microprofile/openapi20/ApplicationProcessor.java
Show resolved
Hide resolved
...file.openapi.2.0.internal/src/io/openliberty/microprofile/openapi20/ApplicationListener.java
Outdated
Show resolved
Hide resolved
...napi.2.0.internal/src/io/openliberty/microprofile/openapi20/css/OpenAPIUIBundlesUpdater.java
Show resolved
Hide resolved
...ile.openapi.2.0.internal/src/io/openliberty/microprofile/openapi20/ApplicationProcessor.java
Show resolved
Hide resolved
OpenApiDocument.INSTANCE.modelFromStaticFile(OpenApiProcessor.modelFromStaticFile(staticFile)); | ||
// OpenApiDocument.INSTANCE.modelFromAnnotations(OpenApiProcessor.modelFromAnnotations(config, IndexUtils.getIndexView(appClassloader, appContainer, config))); | ||
OpenApiDocument.INSTANCE.modelFromAnnotations(OpenApiProcessor.modelFromAnnotations(config, IndexUtils.getIndexView(moduleInfo, config))); |
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.
Another commented out line
* The host specified in VCAP_APPLICATION or null if it is not set. | ||
*/ | ||
@FFDCIgnore(Exception.class) | ||
public static String getVCAPHost() { |
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.
Do you still want to support this?
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.
Good question. Do Liberty instances still run in Cloud Foundry environments? If so, then I guess we need to continue to support it.
However, it does raise the question of what we need to do to ensure that the correct host/port are included in the servers
section when OL is running in OpenShift... sitting behind a service
or, indeed, a route
.
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 is additional behaviour when running in CF. I have no idea whether it's still required. I know we specifically opted not to do anything additional for CF on the reactive messaging feature, for example.
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.
Ok... and we could certainly take that approach for OpenAPI since users are able to specify their own content for the servers
section using a variety of mechanisms.
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.
Ok, I don't mind either way, just as long as we've thought about it and decided what to do, rather than accidentally releasing something which we then have to support forever.
@Version(Constants.OSGI_VERSION) | ||
@TraceOptions(traceGroup = Constants.TRACE_GROUP, messageBundle = Constants.TRACE_OPENAPI) | ||
package io.openliberty.microprofile.openapi20; |
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 isn't how you're supposed to version packages.
Should be 1.0
in the release, with the major and minor versions being updated for future releases in accordance with semantic versioning.
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.
So... simply change Constants::OSGI_VERSION
to be "1.0"?
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.
For now that will do, going forwards, packages are versioned individually, so they're unlikely to have the same version.
There's an awful lot of code in this PR for a feature where we're pulling in a third-party library to do most of the work. |
@Azquelt The majority of the code is the value add functionality that is not part of the MP OpenAPI specification, i.e.:
If we were not continuing to support the value add functionality, we could get rid of approx. 45 java classes. However, even in its current form, there are still significantly less classes than in the previous implementation in the |
Gotcha, that makes sense 👍 |
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 and we've talked through the remaining outstanding points.
#run-libby-bot #build |
Please code review feature-related files, @OpenLiberty/delivery-approvers |
Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_F5W4sO65EeqT3K3gf94lOQ Target locations of links might be accessible only to IBM employees. |
Your Open Liberty build results are ready for viewing.
|
The build msmiths-13589-20200904-1545 For help analyzing your personal build, go to https://cognitive.hursley.ibm.com/buildAnalysis.html?uuid=_F5W4sO65EeqT3K3gf94lOQ |
MP OpenAPI 2.0 Initial Implementation Remove com.ibm.ws.require.java8 bundle from .feature Signed-off-by: Martin Smithson <[email protected]> Specify edition-full in private .feature file Signed-off-by: Martin Smithson <[email protected]> Use the 2.0-RC2 version of MP OpenAPI Signed-off-by: Martin Smithson <[email protected]> Apply PR review comments Signed-off-by: Martin Smithson <[email protected]> Apply more PR review comments Signed-off-by: Martin Smithson <[email protected]>
d45f61d
to
77bbb02
Compare
#run-libby-bot #build |
Please code review feature-related files, @OpenLiberty/delivery-approvers |
Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_dt10oPD1Eeq2j8eW2p4naw Target locations of links might be accessible only to IBM employees. |
Code analysis and actionsDO NOT DELETE THIS COMMENT.
|
Your Open Liberty build results are ready for viewing.
|
The build msmiths-13589-20200907-1146 |
Initial implementation of the
mpOpenAPI-2.0
feature.Associated epic: #11020