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.

Sign up for GitHub

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

Jump to bottom

Move SpotBugs Analysis to Module References #6661

Merged
merged 24 commits into from
Jan 8, 2020
Merged

Move SpotBugs Analysis to Module References #6661

merged 24 commits into from
Jan 8, 2020

Conversation

alzimmermsft
Copy link
Member

Fixes #6660

Moves SpotBugs reporting to use module references instead of package or source code references. Update PR validation to work at the pom.service.xml level instead of pom.client.xml.

@alzimmermsft alzimmermsft self-assigned this Dec 5, 2019
@alzimmermsft
Copy link
Member Author

/azp run java - appconfig - ci

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@alzimmermsft alzimmermsft marked this pull request as ready for review December 5, 2019 21:37
@alzimmermsft
Copy link
Member Author

/azp run java - core - ci

@azure-pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@alzimmermsft alzimmermsft requested review from JimSuplizio and removed request for conniey, mssfang and sima-zhu December 5, 2019 23:57
@mitchdenny
Copy link
Contributor

/azp run java - core

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mitchdenny
Copy link
Contributor

This is good stuff, I think the more we can scope what we are running things against the better!

@alzimmermsft
Copy link
Member Author

Verified that running the aggregate commands on pom.client.xml continue to function as they did before these changes.

@alzimmermsft
Copy link
Member Author

@mitchdenny @JimSuplizio @JonathanGiles @srnagar Could I get a review on the direction and changes made in this PR to validate that they are safe and move us in the direction we want to go with regards to our POM structuring and build.

If need be I will spin off a PR which changes what maven logs during build (removes logging of maven downloading files and Javadoc parsing files) so that we can allow more verbose logging during our PR validation.

mitchdenny
mitchdenny reviewed Dec 20, 2019
View reviewed changes
Copy link
Contributor

@mitchdenny mitchdenny left a comment

Choose a reason for hiding this comment

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

This looks like it is heading in the right direction. If we can make these processes run off the aggregates then I think we are winning and it should also reduce our build times. What is the purpose of spotbugs-aggregate-report/pom.xml moving forward?

eng/pipelines/client.yml Show resolved Hide resolved
eng/pipelines/templates/jobs/archetype-sdk-client.yml Show resolved Hide resolved
@alzimmermsft
Copy link
Member Author

This looks like it is heading in the right direction. If we can make these processes run off the aggregates then I think we are winning and it should also reduce our build times. What is the purpose of spotbugs-aggregate-report/pom.xml moving forward?

Moving forward spotbugs-aggregate-report would be removed for client SDKs, I'm leaving it around as I am uncertain on how the data SDKs leverage it. All that this POM file did was run install and given that the POM itself didn't have any additional build steps added it just ran all install steps of its parent pom.client.xml.

@alzimmermsft
Copy link
Member Author

Current issue still needing to be addressed is that previously the verification of READMEs using the embedme tool used a regex search which ran from the root sdk directory in search of README.md files. This now fails when a module being built and validated doesn't have a README.md file, this is different than previous behavior. This could either be resolved by failing build due to a missing README or having a guard to prevent the check when the README doesn't exist.

@srnagar @hemanttanwar @JimSuplizio Thoughts on the fix for this issue? I am leaning towards failing the build due to a README being missed.

@srnagar
Copy link
Member

srnagar commented Dec 20, 2019 •
edited
Loading

I would suggest making README mandatory for every module. It's a good practice to have even if the module is for internal use only - the README can just document that it's internal. So, yes, failing the build when README is missing is my recommendation.

@alzimmermsft alzimmermsft requested a review from joshfree as a code owner January 3, 2020 23:02
@alzimmermsft alzimmermsft added the EngSys This issue is impacting the engineering system. label Jan 3, 2020
@alzimmermsft
Copy link
Member Author

@srnagar @JonathanGiles @mitchdenny Could I get a review?

JonathanGiles
JonathanGiles reviewed Jan 5, 2020
View reviewed changes
pom.client.xml
@@ -259,6 +259,7 @@
<failOnError>true</failOnError>
<failOnWarnings>true</failOnWarnings>
<doclint>all</doclint>
<quiet>true</quiet>
Copy link
Member

Choose a reason for hiding this comment

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

For other's reference, quiet "Shuts off non-error and non-warning messages, leaving only the warnings and errors appear, making them easier to view."

JonathanGiles
JonathanGiles approved these changes Jan 5, 2020
View reviewed changes
Copy link
Member

@JonathanGiles JonathanGiles left a comment

Choose a reason for hiding this comment

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

This looks fine to me, as long as aggregate reports are still able to be generated and the wiki is updated to represent that correct way to do these.

@mitchdenny mitchdenny mentioned this pull request Jan 8, 2020
Closed
@alzimmermsft alzimmermsft requested a review from samvaity as a code owner January 8, 2020 17:55
@alzimmermsft alzimmermsft merged commit 9ba8f5a into Azure:master Jan 8, 2020
@alzimmermsft alzimmermsft mentioned this pull request Feb 26, 2020
Closed
@alzimmermsft alzimmermsft deleted the AzEng_RemoveSpotbugsDependencies branch March 20, 2020 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JonathanGiles JonathanGiles JonathanGiles approved these changes

@hemanttanwar hemanttanwar hemanttanwar approved these changes

@conniey conniey conniey approved these changes

@JimSuplizio JimSuplizio JimSuplizio approved these changes

@danieljurek danieljurek Awaiting requested review from danieljurek

@weshaggard weshaggard Awaiting requested review from weshaggard

@srnagar srnagar Awaiting requested review from srnagar

@joshfree joshfree Awaiting requested review from joshfree

@mitchdenny mitchdenny Awaiting requested review from mitchdenny

@samvaity samvaity Awaiting requested review from samvaity

Assignees

@alzimmermsft alzimmermsft

Labels
EngSys This issue is impacting the engineering system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Switch SpotBugs to Module Reference
7 participants
@alzimmermsft @mitchdenny @JimSuplizio @srnagar @JonathanGiles @hemanttanwar @conniey