Autorest Regen to update scope to include non-public clouds #39224
Conversation
jairmyree
requested review from
sarangan12,
joshfree,
pvaneck,
KarishmaGhiya,
scottaddie,
gracewilcox and
nisha-bhatia
|
API change check API changes are not detected in this pull request. |
...m/azure/monitor/ingestion/implementation/IngestionUsingDataCollectionRulesClientBuilder.java
Outdated
Show resolved
Hide resolved
...monitor-ingestion/src/main/java/com/azure/monitor/ingestion/models/LogIngestionAudience.java
Outdated
Show resolved
Hide resolved
| // Licensed under the MIT License. | ||
|
|
||
| package com.azure.monitor.ingestion.models; | ||
|
|
...monitor-ingestion/src/main/java/com/azure/monitor/ingestion/models/LogIngestionAudience.java
Outdated
Show resolved
Hide resolved
...monitor-ingestion/src/main/java/com/azure/monitor/ingestion/models/LogIngestionAudience.java
Outdated
Show resolved
Hide resolved
|
|
||
| public class LogIngestionAudience { | ||
|
|
||
| private final String audience; |
There was a problem hiding this comment.
Align the names of the audience with other libraries - for e.g. DocumentAnalysisAudience
| The following edits need to be made manually after code generation: | ||
| - Rollback the edits to `module-info` file |
There was a problem hiding this comment.
Because this is a DPG client, every time you regenerate from swagger it attempts to set the module-info as well. However, it does so incorrectly and will produce build errors if it's not corrected.
There was a problem hiding this comment.
@srnagar After looking at the autorest documentation I didn't see a flag that would disable edits to the module-info.
| implementation-subpackage: "" | ||
| generate-client-as-impl: true |
There was a problem hiding this comment.
Autorest by default will take anything that is to be non-public implementation and put it under a implementation sub-package. But since the namespace for this autorest generation is already com.azure.monitor.ingestion.implementation , we don't want com.azure.monitor.ingestion.implementation.implementation. So you need to specify where to put the implementation which in this case is at the root of the namespace.
There was a problem hiding this comment.
"generate-client-as-impl" is unnecessary and I'll remove that one.
| ```yaml | ||
| java: true | ||
| use: '@autorest/java@4.1.9' |
There was a problem hiding this comment.
We should pin the autorest version that should be used for generating the library.
There was a problem hiding this comment.
@srnagar I'll pin it to the latest, but can you tell me why we should do this? Autorest versions are not auto-updated like other dependencies and I've seen that a lot of other SDKs are using outdated autorest versions because nobody is keeping an eye on them.
There was a problem hiding this comment.
This is because we don't want different devs using different autorest versions to regenerate. If a version is not specified, autorest will use the latest version available locally on the dev's machine which can lead to different code generated on different versions.
| <dependency> | ||
| <groupId>com.azure</groupId> | ||
| <artifactId>azure-monitor-ingestion</artifactId> | ||
| <version>1.2.0-beta.1</version> | ||
| <scope>compile</scope> | ||
| </dependency> | ||
| </dependencies> |
There was a problem hiding this comment.
It's not. I'll remove it.
|
|
||
| import java.util.function.Consumer; | ||
|
|
||
| public class MonitorIngestionCustomizations extends Customization { |
There was a problem hiding this comment.
Add javadoc to describe what customizations are being done here.
…nitor/ingestion/models/LogIngestionAudience.java Co-authored-by: Srikanta <51379715+srnagar@users.noreply.github.com>
…nitor/ingestion/models/LogIngestionAudience.java Co-authored-by: Srikanta <51379715+srnagar@users.noreply.github.com>
…nitor/ingestion/models/LogIngestionAudience.java Co-authored-by: Srikanta <51379715+srnagar@users.noreply.github.com>
sdk/monitor/azure-monitor-ingestion/src/main/java/module-info.java
Outdated
Show resolved
Hide resolved
sdk/monitor/azure-monitor-ingestion/src/main/java/module-info.java
Outdated
Show resolved
Hide resolved
| ```yaml | ||
| java: true | ||
| use: '@autorest/java@4.1.9' |
There was a problem hiding this comment.
This is because we don't want different devs using different autorest versions to regenerate. If a version is not specified, autorest will use the latest version available locally on the dev's machine which can lead to different code generated on different versions.
| output-folder: ../ | ||
| license-header: MICROSOFT_MIT_SMALL | ||
| input-file: https://github.com/Azure/azure-rest-api-specs/blob/main/specification/monitor/data-plane/ingestion/stable/2023-01-01/DataCollectionRules.json | ||
| namespace: com.azure.monitor.ingestion.implementation | ||
| implementation-subpackage: "" |
| } | ||
|
|
||
| /** | ||
| * Customizes the generated code for `IngestionUsingDataCollectionRulesClientBuilder`. |
There was a problem hiding this comment.
Javadoc should describe the customization changes rather than just stating "Customizes "
| import java.util.function.Consumer; | ||
|
|
||
| /** | ||
| * Customization class for Monitor. These customizations will be applied on top of the generated code. |
There was a problem hiding this comment.
The javadoc should include the details of what changes are being made.
Closes #37002
I adjusted the swagger documentation to include the China and US Government clouds as part of the default scopes.
I also adjusted the code generation file to require fewer manual edits upon regeneration. As it is in my PR, you only need to revert the module-info with each regeneration.