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

Refactor [Sonar] #3189

Merged
merged 36 commits into from
Apr 15, 2019
Merged

Refactor [Sonar] #3189

merged 36 commits into from
Apr 15, 2019

Conversation

calebcartwright
Copy link
Member

@calebcartwright calebcartwright commented Mar 11, 2019

Close #1075

Things to note:

  • Our badges will work with the SaaS SonarCloud.io and any other SonarQube deployment that can be reached
    • SonarCloud provides their own badges, but still good to have Shields support for consistent look and feel, customization/enhancement (many of the native SonarCloud badges use a dark grey color regardless of value), etc.
    • Added badge documentation to clearly explain that we work with both
  • The legacy service had the optional version param as a route param prefix. I moved version from route param to query param, with a redirector service to prevent breaking existing badges
    • The Sonar API contract changed dramatically with version 5.4. For prior versions, we need to use the legacy API contract, and any user still using Sonar 5.3 or earlier can get badges working by specifying a value of <= 5.3 for the version param so we know to use the legacy api. The latest version of Sonar is now 7.6 so I believe that the version parameter (only needed for Sonar 5.3 and below) falls under the "not typically used" category, making it a better fit for a query param.
    • Added badge documentation to explain this
  • There was a good chunk of untested code in the legacy service implementation, which I've attempted to address here
    • The Fortify Security Rating badge is using mocked tests, as I don't know of any live endpoint that can be used for test (not available on SonarCloud.io and no known SonarQube deployments with the Fortify plugin installed and in use). I added an inline comment in that tester for future awareness
  • Added support for additional Sonar metrics (our legacy service recognized a few, but for the others would just display the raw value with brightgreen color)
    • Examples of "new" badge support: Quality Gate, Bugs, Code Smells, Duplicated Lines %, Lines of Code, Maintainability, Reliability, Security, Vulnerabilities, and Issues
    • The refactor alone will result in a decent sized diff, so I've decided to push adding these new metrics to future follow ups, and have opened Enhance Sonar Badges #3236 to track that follow up work
    • I went ahead and implemented new support QualityGate badges to serve as a reference

@calebcartwright calebcartwright added the service-badge Accepted and actionable changes, features, and bugs label Mar 11, 2019
@shields-ci
Copy link

shields-ci commented Mar 11, 2019

Warnings
⚠️

📚 Remember to ensure any changes to serverSecrets in services/sonar/sonar-fortify-rating.tester.js are reflected in the server secrets documentation

⚠️

📚 Remember to ensure any changes to serverSecrets in services/sonar/sonar-base.js are reflected in the server secrets documentation

Messages
📖 ✨ Thanks for your contribution to Shields, @calebcartwright!

Generated by 🚫 dangerJS against e006670

@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-3189 March 11, 2019 01:12 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-3189 March 12, 2019 00:27 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-3189 March 12, 2019 00:58 Inactive
@calebcartwright
Copy link
Member Author

@calebcartwright
Copy link
Member Author

Alright I think I've figured out a way in which I can split up refactoring the existing Sonar badges and handling all the others in a follow up. Will open an issue to track that latter one shortly and then finish off the last couple pieces with the refactor here

@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-3189 March 29, 2019 00:45 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-3189 March 29, 2019 02:15 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-3189 March 29, 2019 02:28 Inactive
@calebcartwright
Copy link
Member Author

I know this has been running for a long time (it seems to keep spawning off new threads of work 😄) but I think it's finally just about done.

All that's left is to add the redirector service (to convert version to a query param to maintain support for old SonarQube versions) and update some the badge examples.

I'll mark the PR ready for review once I'm done with those, but it's probably ready now for some new eyes to review if anyone is up for looking at the rest of it.

sizeMetricNames,
testsMetricNames
)
const metricNameRouteParam = metricNames.join('|')
Copy link
Member Author

@calebcartwright calebcartwright Mar 29, 2019

Choose a reason for hiding this comment

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

I elected to enumerate the various metrics in groups based on how Sonar docs typically group those metrics. I've still not had a chance to review each metric in detail, but I think that when implementing #3236 a reasonable starting point would be a service class for each group (I expect there will be a lot of similarity in how the related metrics are handled). Some of those metrics (like the coverage ones) could probably be tacked into one of the existing services

@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-3189 April 14, 2019 00:14 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-3189 April 14, 2019 00:33 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-3189 April 14, 2019 15:04 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-3189 April 14, 2019 15:11 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-3189 April 14, 2019 15:12 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-3189 April 14, 2019 15:57 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-3189 April 14, 2019 16:12 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-3189 April 14, 2019 16:44 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-3189 April 14, 2019 22:54 Inactive
Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

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

I think this is good to merge. Cheers for persisting with it 👍

@calebcartwright
Copy link
Member Author

I think this is good to merge

Thanks for all the feedback!

@shields-ci
Copy link

Warnings
⚠️

📚 Remember to ensure any changes to serverSecrets in services/sonar/sonar-base.js are reflected in the server secrets documentation

⚠️

📚 Remember to ensure any changes to serverSecrets in services/sonar/sonar-fortify-rating.tester.js are reflected in the server secrets documentation

Messages
📖 ✨ Thanks for your contribution to Shields, @calebcartwright!

Generated by 🚫 dangerJS against 3ddd046

@calebcartwright calebcartwright merged commit 700b61e into master Apr 15, 2019
@shields-deployment
Copy link

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch:

@calebcartwright calebcartwright deleted the sonarqube-refactor branch April 15, 2019 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Badge Request: sonarcloud.io
4 participants