Tests for pull request #39 Added license handling using Github API when possible #41
Conversation
|
Running this now and it is looking good :) thanks again @ST-Apps We have a total of 1660 components being tracked across our projects. So it should flex this pretty well. |
|
Fantastic work @ST-Apps and @coderpatros . When its ready for publication, let me know. I can usually get time to do so within a day or two. |
CycloneDX/Services/GithubService.cs
Outdated
| new Regex(@"^https?\:\/\/github\.com\/(?<repositoryId>[^\/]+\/[^\/]+)\/blob\/(?<refSpec>[^\/]+)\/LICENSE(.md)?$"), | ||
| new Regex(@"^https?\:\/\/raw\.githubusercontent\.com\/(?<repositoryId>[^\/]+\/[^\/]+)\/(?<refSpec>[^\/]+)\/LICENSE(.md)?$"), |
There was a problem hiding this comment.
I'm not 100% sure about this, but I think I encountered some LICENSE.txt files in the past, so we may want to allow some other extensions too.
There was a problem hiding this comment.
Interestingly the documentation states it needs to be LICENSE or LICENSE.md. But it also supports LICENSE.txt. Good pick up @ST-Apps.
But what doesn't work, unfortunately, is the ref parameter. Although it's supported, and the response includes the ref in returned urls, the license object is whatever is in master.
I've raised a support request with github about this. But in the meantime I think we'll just return license information when master is referenced. I think that will cover off almost all cases.
|
@stevespringett this should be good to go with one caveat. The GitHub API has a rate limit of 60 requests per hour for unauthenticated requests. With a token the rate limit is 5000 requests per hour. Don't know if you want to merge this now and get it out as is. Or wait until I add an option to provide a token (hopefully tonight). |
|
@stevespringett thinking more about this. It could introduce unpredictable results because of the API throttling. Especially as it is IP based. So a few builds on a build server could quickly use up the unauthenticated quota. So my question is, do you think it should fail “silently” with console output, fail with an error exit code or be opt in functionality? I’m leaning toward opt in by providing a token and failing hard if the 5000 limit is hit. |
|
Current state of this PR is that if a rate limit is hit BoM generation will fail hard. GitHub license resolution will always be attempted, when applicable, and GitHub API credentials are optional. This is probably a reasonable approach. New users can try it out ad hoc without generating a GitHub token. And it also prevents "silent" failures from producing inconsistent BoMs. I'm still running some tests. They normally take about an hour but I suspect they will take a lot longer because of all the extra http calls. But so far this looks pretty solid now. |
|
All looks good now @stevespringett if you want to merge this. |
|
Thanks for completing the PR, I wish I had enough time to be more helpful on this. |
|
@ST-Apps thanks for your PR. It is very helpful. The only urgency on this one was because I want to start using your new functionality straight away :) |
|
@ST-Apps FYI your code is now being used by a government organisation in Australia to help track license compliance and minimise risk for over 200 projects. |
|
That's great! |
|
@stevespringett just bumping this in case it has dropped off your radar :) Summary of new behaviour...
|
|
Thanks for the bump. Been out sick. Merging now. |
This builds on pull request #39, thanks @ST-Apps
I'll run a custom build of this against my 200+ repos for a few days to validate this before switching this PR from draft.
@stevespringett this functionality is really awesome and would be great to get out in the wild. So if there is a particular day/time you can spare to push out a new release let me know. Just so I can make sure I've validated the results and updated this PR before then.