Pico extensibility support for the OCI SDK #6982
Conversation
oracle-contributor-agreement
bot
added
the
OCA Verified
trentjeff
requested review from
tomas-langer,
ljnelson,
romain-grecourt,
danielkec,
spericas and
barchetta
...sdk/processor/src/main/resources/io/helidon/integrations/oci/sdk/processor/no.dot.exceptions
Outdated
Show resolved
Hide resolved
...k/processor/src/main/resources/io/helidon/integrations/oci/sdk/processor/typename.exceptions
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I've left a boatload of comments as you can see.
Since this PR has already been approved I will let its approval status stay. I will leave it up to you what, if any, of my comments you want to address; I certainly don't have all the answers. I think you could let 'er rip and then the issues could be addressed in an ongoing fashion. I hope my comments are clear; I've tried to place myself in the shoes of a prospective user as I read through the 56 files in this PR and tried to capture my reactions as I encountered things.
As you requested, I focused mostly on the AuthenticationDetailsProvider stuff. I think it's generally OK, but I confess I had a little trouble figuring out the configuration and why and how it differs from the current strategies. As long as the user can continue to "do nothing" and the "right" ADP is chosen for her application's runtime environment, that's the most important thing, and, if I'm reading right, that is indeed the case.
At a higher level, I found the language, naming and conceptual structure of all this to be quite loose and in some cases actively surprising, and tried to comment on all the places that were strange or confusing or surprising to me. Some of this was confined to this PR, and some of it is in the overall Helidon Injection core concepts and APIs.
Finally, by not requesting changes on this PR I don't want to be understood to be approving all of Helidon Injection; I think eventually its language, concepts and structures need a hard, careful, thorough, rigorous look, much more documentation, and perhaps even a specification describing exactly what must be true of various implementations of various interfaces and constructs.
Anyway I hope my review is useful and thanks for the work.
| ## Helidon Injection Framework and OCI SDK Integration | ||
| This section only applies for **Helidon SE** type applications. If you are using **Helidon MP** then this section does not apply to you, and you should instead refer to the [cdi](./cdi) module. | ||
|
|
||
| The **Helidon Injection Framework** offers a few different ways to integrate to 3rd party libraries. The **OCI SDK** library, however, is a little different in that a special type/style of fluent builder is needed when using the **OCI SDK**. This means that you can't simply use the _new_ operator when creating instances; you instead need to use the imperative fluent builder style. Fortunately, though, most of the **OCI SDK** follows the same pattern for accessing the API via this fluent builder style. Since the **Helidon Injection Framework** leverages compile-time DI code generation, this arrangement makes it very convenient to generate the correct underpinnings that leverages a template following this fluent builder style. |
There was a problem hiding this comment.
What does "however" modify? Different from what? What is the special style? New instances of what? What is the imperative fluent builder style? Why "fortunately"? What does "though" modify?
"integrate to" -> "integrate with"; "leverages" -> "leverage"
Or better: "Since the Helidon Injection Framework generates code at compile-time..."
I'm not sure what "generate the correct underpinnings that leverages [sic] a template following this fluent builder style" means.
There was a problem hiding this comment.
will improve in a follow-up PR. Keeping this (and other comments below) open as a TODO reminder.
|
|
||
| The **Helidon Injection Framework** offers a few different ways to integrate to 3rd party libraries. The **OCI SDK** library, however, is a little different in that a special type/style of fluent builder is needed when using the **OCI SDK**. This means that you can't simply use the _new_ operator when creating instances; you instead need to use the imperative fluent builder style. Fortunately, though, most of the **OCI SDK** follows the same pattern for accessing the API via this fluent builder style. Since the **Helidon Injection Framework** leverages compile-time DI code generation, this arrangement makes it very convenient to generate the correct underpinnings that leverages a template following this fluent builder style. | ||
|
|
||
| The net of all of this is that there are two modules that you will need to integrate DI into your **Helidon SE** application. |
There was a problem hiding this comment.
What does "module" designate? A Java module?
Did you define what "DI" is?
There was a problem hiding this comment.
Yes, the two modules are listed in the readme, and DI is defined in the top most overview on Pico. What is missing is a link to the top level Pico readme, so keeping this open as a note to add that on the next PR.
|
|
||
| The net of all of this is that there are two modules that you will need to integrate DI into your **Helidon SE** application. | ||
|
|
||
| 1. The [processor](./processor) module is required to be on your compiler / APT classpath. It will observe cases where you are _@Inject_ services from the **OCI SDK** and then code-generate the appropriate [Activator](../api/src/main/java/io/helidon/pico/api/Activator.java)s for those injected services. Remember, the _processor_ module only needs APT classpath during compilation - it is not needed at runtime. |
There was a problem hiding this comment.
(What is "APT"?)
"compiler / APT classpath" -> "compilation classpath", I guess?
What does "observe" mean?
"where you are @Inject services" -> "where you are injecting services"
"code-generate the appropriate Activators" -> "generate appropriate Activator source code
What does "for those injected services" mean? How are Activators and injected services related?
| 2. The [runtime](./runtime) module is required to be on your runtime classpath. This module supplies the default implementation for OCI authentication providers, and OCI extensibility into Helidon. | ||
|
|
||
|
|
||
| ### MP Modules |
There was a problem hiding this comment.
Suggestion for a better choice? It's anything that is "part of the MP flavor", based upon MicroProfile + CDI...
| * @return true if there is sufficient attributes defined for file-based OCI authentication provider applicability | ||
| * @see OciAuthenticationDetailsProvider | ||
| */ | ||
| default boolean fileConfigIsPresent() { |
There was a problem hiding this comment.
Suggest a better name? It is used to determine the availability of the OCI File-based profile scheme.
|
|
||
| /** | ||
| * Determines whether there is sufficient configuration defined in this bean to be used for file-based authentication. This | ||
| * matches to the {@link AuthStrategy#CONFIG_FILE}. |
There was a problem hiding this comment.
i'm not sure what else I could say to make it clearer.
| * Determines whether there is sufficient configuration defined in this bean to be used for file-based authentication. This | ||
| * matches to the {@link AuthStrategy#CONFIG_FILE}. | ||
| * | ||
| * @return true if there is sufficient attributes defined for file-based OCI authentication provider applicability |
There was a problem hiding this comment.
"there is" -> "there are"...not sure frankly what this sentence is saying
|
|
||
| /** | ||
| * Determines whether there is sufficient configuration defined in this bean to be used for simple authentication. This | ||
| * matches to the {@link AuthStrategy#CONFIG}. |
There was a problem hiding this comment.
Same answer - what can make it clearer?
| * @return true if there is sufficient attributes defined for simple OCI authentication provider applicability | ||
| * @see OciAuthenticationDetailsProvider | ||
| */ | ||
| default boolean simpleConfigIsPresent() { |
There was a problem hiding this comment.
Are these methods basically saying, "The user has supplied a strategy descriptor of foo. Let's see if foo could work."? If so, that is not clear to me. Do they need to be public and exposed and therefore managed from version to version, or are they implementation details that could be hidden away somewhere?
There was a problem hiding this comment.
Not really, no. This method (and the other one above) basically ensures that all of the requisite config attributes are indeed present so that it could suffice to be used for the Simple OCI Auth Provider.
Addresses issue #6406 and #6499.