Add new service for CII Best Practices badges [CIIBestPractices] #2477

calebcartwright · 2018-12-08T01:50:00Z

Adds support for badges from CII Best Practices which closes #1530

List of projects can be found at https://bestpractices.coreinfrastructure.org/en/projects for reference

I've never used the service before so some comments/info based on what I've seen:

The CII service provides a rating for a project which is represented with a value of 0-300 in a tiered scale (value is represented as tiered_percentage in the API response)
More details of how CII derives that score can be found here and here, but the important point is that CII natively provides a badge (albeit a non-configurable one) that is ultimately defined by that score
The API response for each service has a field for that raw score value, as well as a grade/level that is defined based on that score:
- level is in_progress for score of 0-99 (inclusive)
- level is passing for score of 100-199 (inclusive)
- level is silver for score of 200-299 (inclusive)
- level is gold for a perfect score of 300
The message of the default badge provided by the service varies both in content and format depending on the value of the level/score (when the score is less than 100 the badge message includes the score percentage with a wide array of colors, otherwise the badge message is simply the level)

As such, I decided to add support for 3 different types of badges for CII Best Practices Data that will give end users a lot more flexibility over the native CII badge:

level - uses the CII level for the badge message with a simple color scheme
percentage uses the CII score/percentage with the standard Shields percentage color scheme
default - uses the same badge message & color scale as the native CII badge provides, but with all the configuration and goodness that Shields provides! (I'm not crazy about default as the path name/label but I wasn't sure what else to call this one)

shields-ci · 2018-12-08T01:50:47Z

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

Generated by 🚫 dangerJS against bd8ab14

PyvesB · 2018-12-10T19:21:41Z

I'll review this shortly. In the meantime, here's the link to a Heroku review app with the contents of this PR deployed.

calebcartwright · 2018-12-10T19:53:40Z

Awesome, thanks @PyvesB!

PyvesB

Unfortunately, the Heroku review app broke in the meantime, we've got something misconfigured somewhere.

Overall this looks like good quality work to me, thank you for your contribution @calebcartwright! I've left some comments on things I believe can be improved. 😉

services/cii-best-practices/cii-best-practices.service.js

services/cii-best-practices/cii-best-practices.tester.js

PyvesB · 2018-12-10T21:16:53Z

services/cii-best-practices/cii-best-practices.tester.js

+  .get(`/level/abc.json`)
+  .expectJSON({ name: 'cii', value: 'project not found' })
+
+t.create('level: unknown project')


I think the live: unknown project test is enough as far as unknown projects are concerned. The bit of code we're testing is the 404: 'project not found', having three additional mocked tests will not improve our coverage nor give us more confidence that the badge is wired up correctly.

Just to clarify, is the suggestion/ask for me to remove those other tests? It doesn't really matter to me either way so just let me know. I agree that the tests won't increase code coverage with the current implementation, but as far as my initial thinking goes - I like to have some tests that validate expected behavior, regardless of what the current implementation code flows allow (more blackbox-ish).

There's 3 different badges and the expected behavior would be project not found for each badge. Absolutely true that with the current implementation that will always be the result regardless of which of the 3 badges are selected (and thus those tests are covering the same lines) but in theory a different implementation of the service could be used in the future where that may not necessarily be true.

Yes, my suggestion was initially to remove all the project not found tests apart from the live one. You're right by saying that we might need additional ones in the future if the implementation changes, however in the meantime they (slightly) slow down our test suite and add code to maintain. ^^

services/cii-best-practices/cii-best-practices.service.js

PyvesB · 2018-12-10T21:40:01Z

services/cii-best-practices/cii-best-practices.service.js

+        keywords,
+      },
+      {
+        title: 'CII Best Practices Default',


Maybe we could make the meaning of "default" clearer by adding some documentation to the example? Pretty much taken from your PR's description: "This badge uses the same message and color scale as the native CII one, but with all the configuration and goodness that Shields provides!". You can refer to our Matrix badge for an example of how to add documentation.

level - uses the CII level for the badge message with a simple color scheme

percentage uses the CII score/percentage with the standard Shields percentage color scheme

default - uses the same badge message & color scale as the native CII badge provides, but with all the configuration and goodness that Shields provides! (I'm not crazy about default as the path name/label but I wasn't sure what else to call this one)

status or summary might be a more descriptive name that emphasizes the content that’s being shown.

Yeah I was having similar thoughts myself, but just wasn't sure what to put in there for the documentation. I like the idea though!

@paulmelnikow - are you suggesting status or summary as a replacement for default (both of which are far better than default) and/or in place of the other two (level or percentage)

i changed default to summary

calebcartwright · 2018-12-11T00:01:30Z

Thanks again for the feedback! Think I addressed everything with the exception of one question to clarify whether some of the tests should be removed.

Also the circleci:danger job is failing on npm install with an interesting conflict error

PyvesB · 2018-12-11T09:00:59Z

Thanks for addressing my review comments! I've clarified the one regarding the project not found tests. I've also kicked the CI server again, that spurious error seems to have gone away. 😉

calebcartwright · 2018-12-11T16:45:11Z

Alrighty then, tests removed as requested

PyvesB

CI checks all green, performed some last sanity checks with a review app. Thanks for your work, let's get this merged! 👍

shields-deployment · 2018-12-11T19:20:55Z

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 · 2018-12-11T19:22:16Z

Excellent, much appreciated. Now I need to see which issues I can work on next!

PyvesB · 2018-12-11T19:31:48Z

@calebcartwright I noticed that you had started a discussion in #2347, maybe it's a good one to tackle? If you're more interested in reviewing something at this point, #2470 is probably a good choice as well. 😉

calebcartwright · 2018-12-11T22:11:58Z

Thanks @PyvesB and yeah the Nexus service one is pretty high on my list (especially since I haven't refactored a legacy service yet), but happy to take a look at the PR as well if it's ready for review.

@paulmelnikow mentioned in another thread that there was plenty of work to do here, so I'm just happy to contribute any way I can. If there's anything in particular I can to do to help please let me know, otherwise I'm just perusing the open issues for ones I think I can work on.

feat: added new service for CII Best Practices badges

aa0299b

paulmelnikow added the service-badge Accepted and actionable changes, features, and bugs label Dec 8, 2018

calebcartwright mentioned this pull request Dec 8, 2018

Fix jenkins cobertura coverage #2346

Closed

PyvesB self-requested a review December 10, 2018 19:01

PyvesB reviewed Dec 10, 2018

View reviewed changes

feat: updates and fixes on new CII Best Practices badges

497cd92

paulmelnikow temporarily deployed to shields-staging-pr-2477 December 10, 2018 23:57 Inactive

Merge branch 'master' into master

b9220b9

paulmelnikow temporarily deployed to shields-staging-pr-2477 December 10, 2018 23:58 Inactive

calebcartwright added 2 commits December 11, 2018 10:34

tests: removed some tests for CII Best Practices service per request

d37613d

Merge branch 'master' of https://github.com/calebcartwright/shields

f21d22d

paulmelnikow temporarily deployed to shields-staging-pr-2477 December 11, 2018 16:39 Inactive

Merge branch 'master' into master

bd8ab14

paulmelnikow deployed to shields-staging-pr-2477 December 11, 2018 16:41 View deployment

PyvesB approved these changes Dec 11, 2018

View reviewed changes

PyvesB merged commit 43faea4 into badges:master Dec 11, 2018

PyvesB mentioned this pull request Feb 19, 2019

Add SymfonyInsight stars badge, run [SymfonyInsight sensiolabs amo chrome-web-store redmine spigetratings vaadin-directory visualstudiomarketplacerating wordpress] #2971

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new service for CII Best Practices badges [CIIBestPractices] #2477

Add new service for CII Best Practices badges [CIIBestPractices] #2477

calebcartwright commented Dec 8, 2018

shields-ci commented Dec 8, 2018 •

edited

Loading

PyvesB commented Dec 10, 2018

calebcartwright commented Dec 10, 2018

PyvesB left a comment

PyvesB Dec 10, 2018

calebcartwright Dec 10, 2018

PyvesB Dec 11, 2018

PyvesB Dec 10, 2018

paulmelnikow Dec 10, 2018

calebcartwright Dec 10, 2018

calebcartwright Dec 11, 2018

calebcartwright commented Dec 11, 2018

PyvesB commented Dec 11, 2018

calebcartwright commented Dec 11, 2018

PyvesB left a comment •

edited

Loading

shields-deployment bot commented Dec 11, 2018

calebcartwright commented Dec 11, 2018

PyvesB commented Dec 11, 2018

calebcartwright commented Dec 11, 2018

Add new service for CII Best Practices badges [CIIBestPractices] #2477

Add new service for CII Best Practices badges [CIIBestPractices] #2477

Conversation

calebcartwright commented Dec 8, 2018

shields-ci commented Dec 8, 2018 • edited Loading

PyvesB commented Dec 10, 2018

calebcartwright commented Dec 10, 2018

PyvesB left a comment

Choose a reason for hiding this comment

PyvesB Dec 10, 2018

Choose a reason for hiding this comment

calebcartwright Dec 10, 2018

Choose a reason for hiding this comment

PyvesB Dec 11, 2018

Choose a reason for hiding this comment

PyvesB Dec 10, 2018

Choose a reason for hiding this comment

paulmelnikow Dec 10, 2018

Choose a reason for hiding this comment

calebcartwright Dec 10, 2018

Choose a reason for hiding this comment

calebcartwright Dec 11, 2018

Choose a reason for hiding this comment

calebcartwright commented Dec 11, 2018

PyvesB commented Dec 11, 2018

calebcartwright commented Dec 11, 2018

PyvesB left a comment • edited Loading

Choose a reason for hiding this comment

shields-deployment bot commented Dec 11, 2018

calebcartwright commented Dec 11, 2018

PyvesB commented Dec 11, 2018

calebcartwright commented Dec 11, 2018

shields-ci commented Dec 8, 2018 •

edited

Loading

PyvesB left a comment •

edited

Loading