mgmt support module-info for resource manager #14127
Conversation
xseeseesee
requested review from
ChenTanyi,
JimSuplizio,
danieljurek,
mitchdenny,
weidongxu-microsoft,
weshaggard and
yungezz
as code owners
| <version>1.1.0</version> <!-- {x-version-update;com.azure:azure-identity;dependency} --> | ||
| <groupId>com.azure.resourcemanager</groupId> | ||
| <artifactId>azure-resourcemanager-test</artifactId> | ||
| <version>2.0.0-beta.4</version> <!-- {x-version-update;com.azure.resourcemanager:azure-resourcemanager-test;current} --> |
There was a problem hiding this comment.
I see that there is apache-commons dependency included. This dependency is not allowed. Can this dependency be removed?
There was a problem hiding this comment.
I think this apache-commons is required for spring cloud service(named appplatform here). It could help on compression and upload.
cc: @ChenTanyi
There was a problem hiding this comment.
Is there any compression package can be used?
There was a problem hiding this comment.
Yes - we should start with the default position that this dependency is not allowed, and work from there to either find an alternative approach or justify why this is needed, but for me the hurdle for accepting this dependency is very high.
| module com.azure.resourcemanager.appplatform { | ||
| requires transitive com.azure.resourcemanager.resources; | ||
| requires transitive com.azure.storage.file.share; | ||
| requires org.apache.commons.compress; |
There was a problem hiding this comment.
If the apache-commons dependency is removed, this should be removed as well.
| requires transitive com.azure.resourcemanager.keyvault; | ||
| requires transitive com.azure.resourcemanager.msi; | ||
| requires transitive com.azure.resourcemanager.dns; | ||
| requires transitive com.azure.resourcemanager.storage; |
There was a problem hiding this comment.
Do these have to be transitive? Will the consumers of azure-resourcemanager-appservice module use the types defined in keyvault, msi, dns and storage modules?
| requires transitive com.azure.resourcemanager.msi; | ||
| requires transitive com.azure.resourcemanager.storage; | ||
| requires transitive com.azure.resourcemanager.network; |
There was a problem hiding this comment.
same here - do these all have to be transitive?
| @@ -0,0 +1,13 @@ | |||
| module com.azure.resourcemanager.appplatform { | |||
There was a problem hiding this comment.
All the module-info.java files are missing license headers.
| opens com.azure.resourcemanager.resources.fluent.inner to com.fasterxml.jackson.databind, com.azure.core; | ||
| opens com.azure.resourcemanager.resources.models to com.fasterxml.jackson.databind, com.azure.core; | ||
|
|
||
| exports com.azure.resourcemanager.resources.fluentcore.arm.collection.implementation |
There was a problem hiding this comment.
Exporting implementation package is not recommended. If there are some common classes that are shared across multiple modules, then those classes should be in non-implementation package and that package should be exported.
There was a problem hiding this comment.
Do you mean we can rename the package and/or the classes? An example is the GroupableResourceImpl used in VirtualMachineImpl. There are a few {CommonResourceModel}Impl shared from azure-resourcemanager-resources to other azure-resourcemanager-** services.
There was a problem hiding this comment.
The intent of the classes inside the 'implementation' packages is that they are never intended for use by end users. In JDK 9+ with modules, we can hide everything under this package and therefore have stronger protections against these being used. If we export them, we're undoing that work. We should have clearly defined barriers between public API and private API - nothing in the public API should expose private API to end users, and so therefore we should never need the user to directly interact with anything in the implementation package. If this is the case today, consider refactoring to get the classes in the correct package and with the correct name.
There was a problem hiding this comment.
@srnagar @JonathanGiles
Would you think if exports implementation to moduleA, moduleB could be acceptable as the end users cannot get them. Alternatively, we may change namespace from com.azure.resourcemanager.resources.fluentcore.arm.models.implementation to com.azure.resourcemanager.resources.fluentcore.arm.models.internal.
There was a problem hiding this comment.
It is best to think of doing 'exports to' as a code smell that should be avoided unless absolutely necessary. It would be preferable to clearly determine firstly if the code is something that should be considered public API or not. That will help to inform how to proceed. It sounds like it should be considered public API to me.
There was a problem hiding this comment.
Agree what you said about The intent of the classes inside the 'implementation' packages is that they are never intended for use by end users. And this is what export to could help for a qualified export. With that, only the designated modules can get access to the 'implementation' packages while end users cannot. It's kind of making them internal available only inside resource manager modules. This is a little bit different comparing to data plane SDK as for resource management these 'implementation' classes are helping model ARM resource in general manner. We will reuse them for the resources among various RPs.
xseeseesee
requested review from
bsiegel,
conniey and
jianghaolu
as code owners
| <!-- Azure-core needs to open the test entities to Jackson for unit tests to operate --> | ||
| --add-opens com.azure.core/com.azure.core.implementation.entities=com.fasterxml.jackson.databind | ||
| --add-opens com.azure.core/com.azure.core.implementation.entities=ALL-UNNAMED | ||
|
|
There was a problem hiding this comment.
IIRC, we were trying to push this into the actual child pom.xml than bloat the parent one. @alzimmermsft, is this correct?
There was a problem hiding this comment.
Yes, we should begin removing library specific configurations from the parent POM. The current configuration results in a lot of warnings from being unable to find modules that exist outside of the project being ran.
|
@conniey The main changes are in the commit of add azure-resourcemanager-test. The rest for each module, it's similar and repeat changes to adopt new test base class. |
Hey. It's easier to reason about changes when they are in smaller pieces, can you create one PR for the addition of the azure-resourcemanager-test and then another for a few modules? Reviewing 300+ effectively is hard and the git UI chokes on it. |
joshfree
added
common
|
Close this as it got separated into smaller PRs. |
9 participants
No description provided.