Update checkstyle rules and suppressions #18025

samvaity · 2020-12-08T21:28:47Z

This PR:

Adds new checkstyle suppression DeclarationOrderCheck, EmptyCatchBlock, OverloadMethodsDeclarationOrderCheck

check-enforcer · 2020-12-08T21:34:31Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run java - [service] - ci

eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml

weidongxu-microsoft

LGTM on resourcemanager packages.

You can ping us if any issue on sdk/<service>/azure-resourcemanager-<service>.

@ChenTanyi @xccc-msft

alzimmermsft · 2020-12-28T17:28:47Z

eng/code-quality-reports/src/main/resources/checkstyle/checkstyle-suppressions.xml

@@ -45,8 +45,25 @@
  <suppress checks="Header" files="com.azure.core.implementation.OptionSerializer"/>
  <suppress checks="Header" files="eventgrid[/\\](models|implementation)[/\\].*\.java"/>

-  <!-- Cryptography Client exception for service client instantiation as it provides client side crypto and is not entirely based on REST service -->
-  <suppress checks="com.azure.tools.checkstyle.checks.ServiceClientCheck" files=".*CryptographyAsyncClient.java"/>
+  <!-- Custom checkstyle rules only check track 2 libraries -->


Based on configuration in parent POMs these checks should never run on Track 1 or management plane, they will only run on modules that use azure-sdk-client-parent as the parent POM.

eng/code-quality-reports/src/main/resources/checkstyle/checkstyle-suppressions.xml

alzimmermsft · 2020-12-28T17:46:57Z

eng/code-quality-reports/src/main/resources/checkstyle/checkstyle-suppressions.xml

+
+  <!-- Package-private helper methods grouped together  -->
+  <suppress checks="OverloadMethodsDeclarationOrderCheck" files=".*[/\\](azure-messaging-servicebus)[/\\].*\.java"/>
+  <suppress checks="OverloadMethodsDeclarationOrderCheck" files="com.azure.storage.file.share.FileSmbProperties.java"/>


Based on what OverloadMethodsDeclarationOrderCheck is we should look at enforcing this in the future as it is a non-breaking change

eng/code-quality-reports/src/main/resources/checkstyle/checkstyle-suppressions.xml

...i-anomalydetector/src/main/java/com/azure/ai/anomalydetector/AnomalyDetectorAsyncClient.java

...anomalydetector/src/main/java/com/azure/ai/anomalydetector/AnomalyDetectorClientBuilder.java

...eys/src/main/java/com/azure/security/keyvault/keys/cryptography/CryptographyAsyncClient.java

alzimmermsft · 2020-12-28T19:51:48Z

eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml

+      2. Instance variables. First the public class variables, then protected, then package level (no access modifier), and then private.
+      3. Constructors
+      4. Methods -->
+    <module name="DeclarationOrder"/>


Is there any configuration options for this check? I'm wondering if we could start with a simplified version where the separate code types, class variables, instance variables, constructors, and methods, need to be group without the more intricate scoping orders.

Yeah I beleive we could skip/ignore access modifiers validation. But still keep the ordering in place with this

<module name="DeclarationOrder"> <property name="ignoreModifiers" value="false" /> </module>

vcolin7

Added a few comments on the checkstyle suppressions and code changes to Key Vault :)

vcolin7 · 2021-01-04T21:22:25Z

eng/code-quality-reports/src/main/resources/checkstyle/checkstyle-suppressions.xml


  <!-- Suppress LineLength for samples and tests -->
  <suppress checks="LineLength" files=".*[/\\](samples|test)[/\\].*\.java"/>

-  <!-- Suppress LineLength for Keyvault on Preview 3. https://github.com/Azure/azure-sdk-for-java/issues/5335 -->
-  <suppress checks="LineLength" files=".*[/\\]com[/\\]azure[/\\]security[/\\]keyvault[/\\]"/>


I am not sure we fixed all line length issues on Key Vault, so if we check this in it will probably fail.

vcolin7 · 2021-01-06T21:30:42Z

...tificates/src/main/java/com/azure/security/keyvault/certificates/CertificateAsyncClient.java

@@ -180,32 +199,19 @@ Duration getDefaultPollingInterval() {
            .flatMap(certificateOperationResponse -> Mono.just(certificateOperationResponse.getValue()));
    }

+    private Function<PollingContext<DeletedCertificate>, Mono<DeletedCertificate>> activationOperation(String certificateName) {


This activation operation is related to the flow for deleting a Certificate, as opposed to the one above which is used for creating one. I think it would be better to just rename them to createCertificateActivationOperation and deleteCertificateActivationOperation and have them live next to the code they're more relevant to.

vcolin7 · 2021-01-07T00:23:13Z

...tificates/src/main/java/com/azure/security/keyvault/certificates/CertificateAsyncClient.java

@@ -1395,6 +1395,15 @@ Duration getDefaultPollingInterval() {
        }
    }

+    Mono<Response<CertificateIssuer>> getIssuerWithResponse(String issuerName, Context context) {


We should probably have getIssuer appear before both getIssuerWithResponse methods like we do with most of our operations.

vcolin7 · 2021-01-07T00:24:25Z

...tificates/src/main/java/com/azure/security/keyvault/certificates/CertificateAsyncClient.java

@@ -1452,6 +1452,15 @@ Duration getDefaultPollingInterval() {
        }
    }

+    Mono<Response<CertificateIssuer>> deleteIssuerWithResponse(String issuerName, Context context) {


Same as above, we should probably have deleteIssuer appear before both deleteIssuerWithResponse methods like we do with most of our operations.

vcolin7 · 2021-01-07T00:37:46Z

...ecurity-keyvault-jca/src/main/java/com/azure/security/keyvault/jca/KeyVaultTrustManager.java

@@ -64,6 +64,16 @@ public KeyVaultTrustManager(KeyStore keyStore) {
        }
    }

+    @Override


We should probably have the overload with less arguments appear before the ones that add to the list. Feels like the more natural progression to me.

vcolin7 · 2021-01-07T00:46:43Z

...t/azure-security-keyvault-keys/src/main/java/com/azure/security/keyvault/keys/KeyClient.java

@@ -403,6 +403,23 @@ public KeyVaultKey getKey(String name, String version) {
        return getKeyWithResponse(name, version, Context.NONE).getValue();
    }

+    /**


This method should appear before the other getKey overload.

vcolin7 · 2021-01-07T00:51:50Z

...ty-keyvault-secrets/src/main/java/com/azure/security/keyvault/secrets/SecretAsyncClient.java

+     * @throws HttpResponseException if {@code name} is empty string.
+     */
+    @ServiceMethod(returns = ReturnType.SINGLE)
+    public Mono<KeyVaultSecret> getSecret(String name) {


We should move this method up to appear before its other overload.

vcolin7 · 2021-01-07T00:52:58Z

...-keyvault-secrets/src/main/java/com/azure/security/keyvault/secrets/SecretClientBuilder.java

    final Map<String, String> properties;
+    private final List<HttpPipelinePolicy> policies;


Is there any reason this is not grouped with the ClientLogger instance below?

vcolin7 · 2021-01-07T01:03:03Z

eng/code-quality-reports/src/main/resources/checkstyle/checkstyle-suppressions.xml

-  <suppress checks="Indentation" files="com.azure.security.keyvault.administration.implementation.RoleAssignmentsImpl"/>
-  <suppress checks="Indentation" files="com.azure.security.keyvault.administration.implementation.RoleDefinitionsImpl"/>


Do these checks no longer fail?

vcolin7 · 2021-01-07T01:05:27Z

eng/code-quality-reports/src/main/resources/checkstyle/checkstyle-suppressions.xml


  <!-- TODO: Synapse access control API does not support PagedResponse and Response. -->
  <suppress checks="com.azure.tools.checkstyle.checks.ServiceClientCheck"
            files="com.azure.analytics.synapse.accesscontrol.AccessControlAsyncClient"/>
-
-  <!-- Public API already exposed: Common vocabulary errors, refer to Java Spec: https://azure.github.io/azure-sdk/java_introduction.html,


Was the checkstyle issue flagging these KeyVault vocabulary violations fixed?

samvaity · 2021-02-19T00:37:39Z

Closing this to focus on including smaller changes.

Dev sentinel 2022 01 01 preview (Azure#18025) * Adds base for updating Microsoft.SecurityInsights from version preview/2021-10-01-preview to version 2022-01-01-preview * Updates readme * Updates API version in new specs and examples * SourceControl API 2022-01-01-preview version - adding new attribute to SourceControl model (Azure#17849) * SourceControl 2022-01-01-preview version change * fix validation error * prettified * prettified Co-authored-by: debjyoti11 <[email protected]> * New metadata API (Azure#17942) * metadata changes * addressed changes * updated enum kind * updated values as well Co-authored-by: Roey Ben Chaim <[email protected]> * added new enum to kind (Azure#17994) Co-authored-by: Roey Ben Chaim <[email protected]> * Add flag to rotate webhook secret (Azure#17989) Co-authored-by: debjyoti11 <[email protected]> * Align definitions with stable version (Azure#17931) * Align definitions with stable version * align scheduled rule and template properties with stable * Note: We are creating a new api-version with this change. Watchlists.json 2022-01-01-preview parameter name changes (Azure#17848) * changed source to sourceType and watchlistAlias to alias * Update CreateWatchlistAndWatchlistItems.json * Update CreateWatchlistItem.json * Update DeleteWatchlist.json * Update DeleteWatchlistItem.json * Update GetWatchlistByAlias.json * Update GetWatchlistItemById.json * Update GetWatchlistItems.json * Update GetWatchlistByAlias.json * added missing params to match stable version * Revert "added missing params to match stable version" This reverts commit a30a709613f94266d0eaba6495c92be43221a282. Reverting by 1 commit * refreshing pipeline * reverting lint diff change * attempting to fix RemovedPath breaking changes * Revert "attempting to fix RemovedPath breaking changes" This reverts commit 2f96ee3c050fde4c166bf730b0a1b5faff07cfc1. reverting attempt at correcting breaking changes. cannot match preview to stable without breaking everything. * adding back source type for backwards compatibility, added response descriptions, corrected PR comments * adding missing bracket * ran npm run prettier-fix to correct style issues * resolving model/definition errors * Revert "resolving model/definition errors" This reverts commit fd67a4f813d923965cefad50999010820446a107. Reverting prettier changes made to other branches * Revert "ran npm run prettier-fix to correct style issues" This reverts commit 3055791e00e956fff3a62e24d8313dde96daa00c. * update * Update DeleteWatchlist.json * Update Watchlists.json * Update DeleteWatchlist.json * Update DeleteWatchlist.json * Update DeleteWatchlist.json * Update DeleteWatchlist.json * Update DeleteWatchlist.json * Update DeleteWatchlist.json * fixing styling issues on Delete example * Update DeleteWatchlist.json * Update DeleteWatchlist.json * ran npm run prettier on DeleteWatchlist.json * Update Watchlists.json * reverting alias to watchlistAlias Co-authored-by: Hassan Chawiche <[email protected]> * Rename system to AutoAssigned (Azure#18061) * Rename system to AutoAssigned * Add example to list operations * Update API version in NRT examples * Execute validation tools and fix results (Azure#17733) * Execute validation tools and fic results * Prettyfieir GetIoTById.json * Watchlists.json-2022-01-01-preview-parameterNameChanges (reverted PR reference for files changed -> https://github.com/Azure/azure-rest-api-specs/pull/17848/files) (Azure#18085) * changed source to sourceType and watchlistAlias to alias * Update CreateWatchlistAndWatchlistItems.json * Update CreateWatchlistItem.json * Update DeleteWatchlist.json * Update DeleteWatchlistItem.json * Update GetWatchlistByAlias.json * Update GetWatchlistItemById.json * Update GetWatchlistItems.json * Update GetWatchlistByAlias.json * added missing params to match stable version * Revert "added missing params to match stable version" This reverts commit a30a709613f94266d0eaba6495c92be43221a282. Reverting by 1 commit * refreshing pipeline * reverting lint diff change * attempting to fix RemovedPath breaking changes * Revert "attempting to fix RemovedPath breaking changes" This reverts commit 2f96ee3c050fde4c166bf730b0a1b5faff07cfc1. reverting attempt at correcting breaking changes. cannot match preview to stable without breaking everything. * adding back source type for backwards compatibility, added response descriptions, corrected PR comments * adding missing bracket * ran npm run prettier-fix to correct style issues * resolving model/definition errors * Revert "resolving model/definition errors" This reverts commit fd67a4f813d923965cefad50999010820446a107. Reverting prettier changes made to other branches * Revert "ran npm run prettier-fix to correct style issues" This reverts commit 3055791e00e956fff3a62e24d8313dde96daa00c. * update * Update DeleteWatchlist.json * Update Watchlists.json * Update DeleteWatchlist.json * Update DeleteWatchlist.json * Update DeleteWatchlist.json * Update DeleteWatchlist.json * Update DeleteWatchlist.json * Update DeleteWatchlist.json * fixing styling issues on Delete example * Update DeleteWatchlist.json * Update DeleteWatchlist.json * ran npm run prettier on DeleteWatchlist.json * Update Watchlists.json * reverting alias to watchlistAlias * fixing type version * Update Watchlists.json * Update DeleteWatchlist.json * fixing commas Co-authored-by: Hassan Chawiche <[email protected]> Co-authored-by: Usman Hasan <[email protected]> * Done (Azure#17843) Co-authored-by: Roy Reinhorn <[email protected]> * AutomationRules 2022-01-01-preview: Add constraints (Azure#18099) * add contraints (alinged with the latest stable version) Co-authored-by: debroy <[email protected]> Co-authored-by: debjyoti11 <[email protected]> Co-authored-by: RoeyBC <[email protected]> Co-authored-by: Roey Ben Chaim <[email protected]> Co-authored-by: Laith Hisham <[email protected]> Co-authored-by: ushasan <[email protected]> Co-authored-by: Hassan Chawiche <[email protected]> Co-authored-by: shschwar <[email protected]> Co-authored-by: Usman Hasan <[email protected]> Co-authored-by: royrein <[email protected]> Co-authored-by: Roy Reinhorn <[email protected]> Co-authored-by: dosegal <[email protected]> Co-authored-by: Arthur Ning <[email protected]>

samvaity force-pushed the checkstyle-updates branch 3 times, most recently from 6ddd5be to dc2eef7 Compare December 9, 2020 19:30

samvaity commented Dec 15, 2020

View reviewed changes

eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml Outdated Show resolved Hide resolved

JonathanGiles reviewed Dec 15, 2020

View reviewed changes

eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml Outdated Show resolved Hide resolved

samvaity force-pushed the checkstyle-updates branch 3 times, most recently from 2b762cc to e6e012d Compare December 21, 2020 10:34

samvaity force-pushed the checkstyle-updates branch 4 times, most recently from c82413f to 79fa5d4 Compare December 24, 2020 06:42

samvaity marked this pull request as ready for review December 24, 2020 07:59

samvaity requested review from alzimmermsft, amishra-dev, ankitarorabit, anuchandy, arerlend, backwind1233, bsiegel, chenrujun, ChenTanyi, conniey, danieljurek, FabianMeiswinkel, g2vinay, gapra-msft, hemanttanwar and jaschrep-msft as code owners December 24, 2020 07:59

samvaity requested review from srnagar, vcolin7, weidongxu-microsoft, xseeseesee, xinlian12, YijunXieMS, yiliuTo, yungezz, yvgopal and a team as code owners December 24, 2020 07:59

weidongxu-microsoft approved these changes Dec 24, 2020

View reviewed changes

saragluna mentioned this pull request Dec 24, 2020

Enable DeclarationOrder check for azure-spring package related files #18291

Closed

alzimmermsft reviewed Dec 28, 2020

View reviewed changes

samvaity marked this pull request as draft December 30, 2020 13:16

samvaity added 4 commits December 30, 2020 12:42

update checkstyle and supppressions

e93f8a9

update to latest version

7d30d52

add more checks

a0e1862

update checkstyle files

531bb76

samvaity force-pushed the checkstyle-updates branch 2 times, most recently from 8fa130e to 0c8af3d Compare December 30, 2020 22:32

cleanup java files

0c720f0

samvaity force-pushed the checkstyle-updates branch from 0c8af3d to 0c720f0 Compare December 30, 2020 23:33

vcolin7 reviewed Jan 7, 2021

View reviewed changes

samvaity mentioned this pull request Jan 12, 2021

Enable ThrowFromClientLoggerCheck for track 2 libraries #18399

Closed

6 tasks

joshfree added EngSys This issue is impacting the engineering system. Infrastructure labels Jan 13, 2021

samvaity closed this Feb 19, 2021

samvaity deleted the checkstyle-updates branch June 22, 2021 20:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update checkstyle rules and suppressions #18025

Update checkstyle rules and suppressions #18025

samvaity commented Dec 8, 2020 •

edited

Loading

check-enforcer bot commented Dec 8, 2020

weidongxu-microsoft left a comment

alzimmermsft Dec 28, 2020

alzimmermsft Dec 28, 2020

alzimmermsft Dec 28, 2020

samvaity Dec 30, 2020

vcolin7 left a comment

vcolin7 Jan 4, 2021

vcolin7 Jan 6, 2021

vcolin7 Jan 7, 2021

vcolin7 Jan 7, 2021

vcolin7 Jan 7, 2021

vcolin7 Jan 7, 2021

vcolin7 Jan 7, 2021

vcolin7 Jan 7, 2021

vcolin7 Jan 7, 2021

vcolin7 Jan 7, 2021

samvaity commented Feb 19, 2021 •

edited

Loading

		final Map<String, String> properties;
		private final List<HttpPipelinePolicy> policies;

		<suppress checks="Indentation" files="com.azure.security.keyvault.administration.implementation.RoleAssignmentsImpl"/>
		<suppress checks="Indentation" files="com.azure.security.keyvault.administration.implementation.RoleDefinitionsImpl"/>

Update checkstyle rules and suppressions #18025

Update checkstyle rules and suppressions #18025

Conversation

samvaity commented Dec 8, 2020 • edited Loading

check-enforcer bot commented Dec 8, 2020

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

weidongxu-microsoft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcolin7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samvaity commented Feb 19, 2021 • edited Loading

samvaity commented Dec 8, 2020 •

edited

Loading

samvaity commented Feb 19, 2021 •

edited

Loading