-
Notifications
You must be signed in to change notification settings - Fork 2.1k
mgmt support module-info for resource manager #14127
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
Changes from all commits
741fd19
80db3dc
4a117fc
b829c16
d6acbb2
c24c274
2c750fc
646412c
0380f8d
2c8f0e9
36b7bbc
72305d2
53ea270
100056a
7950075
0daf905
c50a6b1
47df98d
3a50c55
7962298
0fecfea
417766a
e4ffa0d
cf3fda6
1ae3f60
7ac2fed
e0f51d0
b843d8b
513b965
054215f
cfd0166
fb84736
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,9 +83,9 @@ | |
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>com.azure</groupId> | ||
| <artifactId>azure-identity</artifactId> | ||
| <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} --> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that there is apache-commons dependency included. This dependency is not allowed. Can this dependency be removed?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this apache-commons is required for spring cloud service(named appplatform here). It could help on compression and upload. cc: @ChenTanyi
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any compression package can be used?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| module com.azure.resourcemanager.appplatform { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All the module-info.java files are missing license headers. |
||
| requires transitive com.azure.resourcemanager.resources; | ||
| requires transitive com.azure.storage.file.share; | ||
| requires org.apache.commons.compress; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the apache-commons dependency is removed, this should be removed as well. |
||
|
|
||
| exports com.azure.resourcemanager.appplatform; | ||
| exports com.azure.resourcemanager.appplatform.fluent; | ||
| exports com.azure.resourcemanager.appplatform.fluent.inner; | ||
| exports com.azure.resourcemanager.appplatform.models; | ||
|
|
||
| opens com.azure.resourcemanager.appplatform.fluent.inner to com.fasterxml.jackson.databind, com.azure.core; | ||
| opens com.azure.resourcemanager.appplatform.models to com.fasterxml.jackson.databind, com.azure.core; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| module com.azure.resourcemanager.appservice { | ||
| requires transitive com.azure.resourcemanager.keyvault; | ||
| requires transitive com.azure.resourcemanager.msi; | ||
| requires transitive com.azure.resourcemanager.dns; | ||
| requires transitive com.azure.resourcemanager.storage; | ||
|
Comment on lines
+2
to
+5
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do these have to be transitive? Will the consumers of |
||
|
|
||
| exports com.azure.resourcemanager.appservice; | ||
| exports com.azure.resourcemanager.appservice.fluent; | ||
| exports com.azure.resourcemanager.appservice.fluent.inner; | ||
| exports com.azure.resourcemanager.appservice.models; | ||
|
|
||
| opens com.azure.resourcemanager.appservice.fluent.inner to com.fasterxml.jackson.databind, com.azure.core; | ||
| opens com.azure.resourcemanager.appservice.models to com.fasterxml.jackson.databind, com.azure.core; | ||
| } | ||
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.