Conversation
67592db to
1d3504e
Compare
|
@Bobgy this PR is ready for review. |
|
@Bobgy Is there something I can do still to get this merged? |
Sorry I am not spending enough time on this. I tried looking at it initially, but some part of the code was a bit hard to understand. I don't remember now. I think the simplest way to give us enough confidence is by setting up a bigger e2e test example and confirm the result has no diffence for this PR. |
949bfd3 to
3008ef3
Compare
|
Thank you! I will take another quick look |
Bobgy
left a comment
There was a problem hiding this comment.
Generally looks great! I need to read library.go again not on my phone.
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
|
@Bobgy Is this PR ready to be merged? |
|
Thought i'd try this against our production code ahead of merge. It's certainly faster.. 5 seconds vs 27 seconds, however it initially failed to complete for our code as It also picked up a license which master didn't before for Will dig in further as time permits.. aside from the There seems to be a change in behaviour for templated output, however.. my template looks like this: This now seems to range over each license for each package, whereas previously it looped over each package once. This means that for packages that have three licenses, it will emit Will dig in further when i get a bit of time |
|
Could you verify that the Also, on master, there has been a change that adds all licenses in a LICENSE file to the report. That might explain your second issue where it shows multiple entries for the same file? Please take a look at the contents of that LICENSE file and check if it contains multiple licenses. @gwatts thanks for the testing! |
|
I just created a new throwaway Go project here with a package that imports With github.com/gwatts/rootcerts,https://github.com/gwatts/rootcerts/blob/ff6b9d3bf583/LICENSE,MIT
github.com/jmespath/go-jmespath,https://github.com/jmespath/go-jmespath/blob/v0.4.0/LICENSE,Apache-2.0As of commit e41525c though it doesn't output And for this PR branch: |
|
@gwatts https://github.com/jmespath/go-jmespath/blob/v0.4.0/LICENSE not being recoginised is due to a change in the behavior of licenseclassifier/v2 vs licenseclassifier, and I don't think we can do much about it. It also looks like the particular LICENSE file has been updated to a more uniform format: jmespath/go-jmespath@b89213a Thanks to pointing out that the In this branch, I moved the "no licenses found" check out of the I think we should actually show an error and add the Unknown entry because we did not find a license. This PR by accident fixes the bug. We should however improve the error message ( |
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
|
Thank you @inteon for the awesome perf improvements! It's great to see the test numbers. Let's merge and release v2 alpha first. |
Greatly improves performance of the go-licenses tool by preventing a duplicate expensive operations and parallelising them.
The performance improvement for cert-manager: