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

Add [ClearlyDefined] service #6944

Merged
merged 23 commits into from
Sep 29, 2021
Merged

Add [ClearlyDefined] service #6944

merged 23 commits into from
Sep 29, 2021

Conversation

mbtools
Copy link
Contributor

@mbtools mbtools commented Aug 25, 2021

Description

This service is for displaying the overall score of repositories as defined by ClearlyDefined. ClearlyDefined is part of the Open Source Initiative, and on a mission to help FOSS projects thrive by being, well, clearly defined. For more details visit
https://clearlydefined.io/about

The badge will display the overall score out of 100 for a given repository. The badge describes how well licenses are defined. Since it's basically an analysis of license metadata and it has been placed under the "Analysis" category.

Example of a repository on ClearlyDefined:
https://clearlydefined.io/definitions/npm/npmjs/-/jquery/3.4.1
The badge will display the top right number on the screenshot using the "score/100" format. For the given example, the badge will show "Score: 88/100".

Corresponding API call:
https://api.clearlydefined.io/definitions/npm/npmjs/-/jquery/3.4.1

image

Data

The public API is available as Swagger UI and does not require any API key:
https://api.clearlydefined.io/api-docs/

Motivation

The badge shall promote the ClearlyDefined initiative and help to bring more clarity around licenses in the open source community. You can find more details at https://docs.clearlydefined.io/.

@shields-ci
Copy link

shields-ci commented Aug 25, 2021

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

Generated by 🚫 dangerJS against debe1ea

@lgtm-com
Copy link

lgtm-com bot commented Aug 25, 2021

This pull request introduces 1 alert when merging df54dc1 into 22995e4 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Aug 26, 2021

This pull request introduces 1 alert when merging bccf927 into 22995e4 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@calebcartwright calebcartwright added the service-badge Accepted and actionable changes, features, and bugs label Aug 26, 2021
Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. More context and content would've been helpful to get a better picture of the objective/motivation, including the questions asked in our issue template for new badges.

I've gone ahead and taken a pass through this anyway, and left several questions and requested changes inline below that will need to be resolved before we could potentially move ahead.

services/clearlydefined/clearlydefined-base.js Outdated Show resolved Hide resolved
services/clearlydefined/clearlydefined-base.js Outdated Show resolved Hide resolved
services/clearlydefined/clearlydefined-score.service.js Outdated Show resolved Hide resolved
services/clearlydefined/clearlydefined-base.js Outdated Show resolved Hide resolved
services/clearlydefined/clearlydefined-score.service.js Outdated Show resolved Hide resolved
services/clearlydefined/clearlydefined-score.service.js Outdated Show resolved Hide resolved
services/clearlydefined/clearlydefined-base.js Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Aug 27, 2021

This pull request introduces 1 alert when merging aae9b82 into 22995e4 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@mbtools
Copy link
Contributor Author

mbtools commented Aug 27, 2021

Thanks for the quick the thorough feedback!

I had used the default PR template and missed that there was a special one for new services. I updated the description accordingly.

I will dig through the linting and testing now...

@mbtools
Copy link
Contributor Author

mbtools commented Aug 27, 2021

I apologize but I can't figure out why it complains about "prettier". It runs on my vscode and shows no issues in the files for this PR.

Also, where do we store the json results for test API calls or are those generated?

@calebcartwright
Copy link
Member

I apologize but I can't figure out why it complains about "prettier". It runs on my vscode and shows no issues in the files for this PR.

The CI check runs prettier using the version of prettier that's specified in the project manifest. I'm not quite positive what you meant with this comment, but it sounds like you're referring to the Prettier plugin for VS Code?

What do you see when you run the same script in your local workspace? npm run prettier:check

Also, where do we store the json results for test API calls or are those generated?

I don't understand the question. Are you referring to the mocha test results, or the json response that an integration/service test receives when calling the running server? You should be able to view the test results within the Circle CI job, e.g. https://app.circleci.com/pipelines/github/badges/shields/8507/workflows/d089bb9b-7a26-4b14-8944-917e8e5ff493/jobs/154701

And if your question is referring to the latter, we don't persist the API responses generated during test runs anywhere on disk. As of 7f608c1 the service tests are missing the required .json extension, which means the response is coming back as an svg (which is the default). The test validators are expecting a json response, which is why the tests are failing with errors about not being able to parse the xml of the svg as json.

@shields-cd shields-cd temporarily deployed to shields-staging-pr-6944 August 28, 2021 02:55 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6944 August 28, 2021 15:46 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6944 August 28, 2021 15:58 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6944 August 28, 2021 16:11 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6944 August 28, 2021 16:14 Inactive
@mbtools
Copy link
Contributor Author

mbtools commented Aug 29, 2021

The second case exists in the ClearlyDefined data. It is rated 0. So we get a valid json response. Kind of the same as first case so we could remove it.

I was expecting 404 for the third case as well. But it returns a json with an error code.

@calebcartwright
Copy link
Member

So I grabbed the ref for this PR and am working with it locally, and one of the things that concerns me is the how long the ClearlyDefined API takes to respond, at least on the first call.

The badges our service provides are primarily displayed in places like GitHub readmes which enforce a fairly narrow round trip window in order for things like our badges (and any image/fetched content) to render. If the entire request/response workflow doesn't complete within ~3-4 seconds then GitHub won't display the result. It seems like the initial request for a given package revision can take north of 10 seconds, at least in my anecdotal experience.

Do you know if it's performing the entire analysis on demand when it receives the request? If so then I don't think this badge is going to be viable.

@mbtools
Copy link
Contributor Author

mbtools commented Aug 31, 2021

Hmm. My current understanding is that the analysis happens when repos are harvested by ClearlyDefined. The API call shouldn't take long but then who knows how many people have actually tested this. I will request details from the CD folks. Let's put this PR into draft until we have clarity because I agree, it needs to be reasonably fast

@mbtools
Copy link
Contributor Author

mbtools commented Sep 2, 2021

The API now respondes in 0.5 and 1.5sec. Looks ok with my tests (from Canada).

@shields-cd shields-cd temporarily deployed to shields-staging-pr-6944 September 4, 2021 17:54 Inactive
@calebcartwright
Copy link
Member

The API now respondes in 0.5 and 1.5sec. Looks ok with my tests (from Canada).

Thanks for following up, it does indeed seem much more snappy now. The error behavior is still a bit of concern for me. Here's my observations, let me know if you expect and/or see anything different:

  • Invalid/unknown revision: HTTP 200 response with a score of 0
  • Invalid/unknown package: HTTP 200 response with a score of 0
  • Invalid/unknown namespace: HTTP 200 response with a score of 0
  • Invalid/unknown provider: HTTP 500 response
  • Invalid/unknown type: HTTP 500 response

That's not necessarily a blocker, but I'd suggest that's a non-ideal response paradigm for an API and complicates our error handling and presentation to users. We could handle the 500 cases and set the message to be something like unknown type/provider, or upstream issue (with the latter to account for genuine internal server type errors unrelated to the bad params), but is there anything possible with the other 3 bad input scenarios? Is a score of 0 possible with valid inputs?

@mbtools
Copy link
Contributor Author

mbtools commented Sep 10, 2021

No response yet clearlydefined/service#870

@calebcartwright
Copy link
Member

No response yet clearlydefined/service#870

Thanks so much for following up on that! I suggest we hold off for a bit to see what comes out of that, but if there's no news in the near future we can go ahead and proceed.

@calebcartwright
Copy link
Member

I'm tracking along with the upstream discussion, and while it's good to see an active dialog, I'm also getting the sense that's not anything that's likely to change any time soon.

We definitely wouldn't want to introduce an extra call and sequential call chain for our purposes, so I think we'll be stuck with the existing behavior as-is. What do you think?

@mbtools
Copy link
Contributor Author

mbtools commented Sep 21, 2021

It certainly will take a while to sort this on their side, especially since it could be an incompatible change. Let's assume it stays as-is.

The 200 cases are missing certain parts in the JSON. So I can check this and return "not found" for the badge. I'm not sure if I can handle the 500 or if this requires a change somewhere else.

mbtools and others added 4 commits September 21, 2021 10:44
Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

One last item on the new error handling scenario, then we'll be good to merge! Thanks again for all your and sticking with this even though it had to spawn off some upstream threads

services/clearlydefined/clearlydefined-score.service.js Outdated Show resolved Hide resolved
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6944 September 25, 2021 03:01 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6944 September 28, 2021 14:50 Inactive
@mbtools
Copy link
Contributor Author

mbtools commented Sep 28, 2021

I think we got it. Thanks for your help!

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

Excellent, thank you!

@mbtools
Copy link
Contributor Author

mbtools commented Sep 29, 2021

Awesome. I don't see it on https://shields.io/category/analysis yet but I guess there's some follow-up on your end to make it productive.

@calebcartwright
Copy link
Member

Awesome. I don't see it on https://shields.io/category/analysis yet but I guess there's some follow-up on your end to make it productive.

Correct. We do automatically deploy merged changes through our staging environment (https://shields-staging.herokuapp.com/), but we don't auto-deploy straight to prod. We typically deploy to prod at least once a week, so odds are pretty high this will be live by Monday if not earlier.

@calebcartwright
Copy link
Member

@mbtools mbtools deleted the clearlydefined branch October 3, 2021 13:45
@mbtools
Copy link
Contributor Author

mbtools commented Oct 3, 2021

Woohoo! Works well. 😄
I put one on my repo here (my other repos are still private)
https://github.com/Marc-Bernard-Tools/Marc-Bernard-Tools-Versions

It was a great learning experience for me. Thanks for your help.

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.

4 participants