Skip to content
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

Update Security Libraries to New Codesnippet Tooling #25441

Merged

Conversation

alzimmermsft
Copy link
Member

Description

Fixes #7154
Fixes #23845
Fixes #24975
Fixes #24976
Fixes #24977
Fixes #24978
Fixes #24979

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@alzimmermsft alzimmermsft added the Client This issue points to a problem in the data-plane of the library. label Nov 16, 2021
@alzimmermsft alzimmermsft self-assigned this Nov 16, 2021
@ghost ghost added KeyVault azure-spring All azure-spring related issues labels Nov 16, 2021
Comment on lines +43 to +44
<javadocDoclet></javadocDoclet>
<javadocDocletOptions></javadocDocletOptions>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having empty tags seems a bit odd and is done in almost all libraries. Can we invert this pattern where libraries having a non-empty value would override what's in parent pom? (Of course, this might be easier after majority of the libraries have migrated to the new tool)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These empty lines will go away once the codesnippet tooling transition is complete. They're empty as the configuration in the parent POM is to enable the previous codesnippet doclet and this empty configuration disables it.

Comment on lines +28 to +30
/**
* Algorithm.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this enforced by the javadoc plugin?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I believe something changed where all fields in a serializable class need to have Javadocs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a private field and requiring javadocs on private fields can become tedious. Is there a way to turn this off while generating javadocs?

@@ -85,7 +84,7 @@ private static Path getConfiguredKeyStorePath() {

@SuppressWarnings("removal")
private static String privilegedGetProperty(String theProp, String defaultVal) {
return AccessController.doPrivileged(
return java.security.AccessController.doPrivileged(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to this file can be reverted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this is a compiler warning fix for Java 17. Apparently when you suppress warnings for removal, or anything, on the class it doesn't cover import statements and import statements can't be annotated with SuppressWarnings. Making the usage of the class fully-qualified allows for the method to suppress the warning and fix the compilation warning.

@alzimmermsft
Copy link
Member Author

/check-enforcer override

@alzimmermsft
Copy link
Member Author

Test was failing due to a known issue #25422

@alzimmermsft alzimmermsft merged commit 6df6a8e into Azure:main Nov 16, 2021
@alzimmermsft alzimmermsft deleted the AzSecurity_UpdateToNewCodesnippetTooling branch November 16, 2021 20:04
Copy link
Member

@vcolin7 vcolin7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this is already merged, just left a couple comments :)

Comment on lines +43 to +44
<javadocDoclet></javadocDoclet>
<javadocDocletOptions></javadocDocletOptions>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason we add these empty properties?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These empty lines will go away once the codesnippet tooling transition is complete. They're empty as the configuration in the parent POM is to enable the previous codesnippet doclet and this empty configuration disables it.

Comment on lines +24 to +26
<codesnippet.skip>false</codesnippet.skip>
<javadocDoclet></javadocDoclet>
<javadocDocletOptions></javadocDocletOptions>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this in this project or other perf projects if we are not going to make use of the codesnippets tool in it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until the previous codesnippet tooling is replaced, yes as it will still attempt to run during Javadoc generation

XiaofeiCao pushed a commit to XiaofeiCao/azure-sdk-for-java that referenced this pull request Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
azure-spring All azure-spring related issues Client This issue points to a problem in the data-plane of the library. KeyVault
Projects
None yet
3 participants