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

FOSSA scanning enabled for the repo #2347

Merged
merged 7 commits into from
Aug 29, 2020
Merged

FOSSA scanning enabled for the repo #2347

merged 7 commits into from
Aug 29, 2020

Conversation

idvoretskyi
Copy link
Contributor

Fixes #854

Signed-off-by: Ihor Dvoretskyi ihor@linux.com

@idvoretskyi idvoretskyi requested a review from a team as a code owner July 16, 2020 19:26
@idvoretskyi idvoretskyi requested a review from jpkrohling July 16, 2020 19:26
@idvoretskyi idvoretskyi mentioned this pull request Jul 16, 2020
7 tasks
Signed-off-by: Ihor Dvoretskyi <ihor@linux.com>
@codecov
Copy link

codecov bot commented Jul 16, 2020

Codecov Report

Merging #2347 into master will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2347      +/-   ##
==========================================
- Coverage   95.58%   95.57%   -0.01%     
==========================================
  Files         208      208              
  Lines       10682    10682              
==========================================
- Hits        10210    10209       -1     
  Misses        401      401              
- Partials       71       72       +1     
Impacted Files Coverage Δ
cmd/query/app/static_handler.go 84.16% <0.00%> (-1.67%) ⬇️
plugin/storage/integration/integration.go 80.86% <0.00%> (+0.47%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9aada2c...ea3dede. Read the comment docs.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Seems that it also timed out. When it says "still being analyzed on fossa.com", is the analysis done locally in CI or remotely via FOSSA?

# Runs a set of commands to initialize and analyze with FOSSA
- name: run FOSSA analysis
env:
FOSSA_API_KEY: '304657e2357ba57b416b94e6b119131b'
Copy link
Member

Choose a reason for hiding this comment

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

What is the origin of this key? Please add a comment describing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yurishkuro it's a FOSSA's push-only API token - https://docs.fossa.com/docs/api-reference#push-only-api-token; which is safe to be exposed publicly.

Copy link
Member

Choose a reason for hiding this comment

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

but where does it come from? Is this ID specific to Jaeger project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it was created for Jaeger only.

- uses: actions/checkout@v2
- uses: actions/setup-go@v2
with:
go-version: "^1.13.1"
Copy link
Member

Choose a reason for hiding this comment

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

will it take 1.14.x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yurishkuro let's try :)

Signed-off-by: Ihor Dvoretskyi <ihor@linux.com>
@idvoretskyi
Copy link
Contributor Author

Seems that it also timed out. When it says "still being analyzed on fossa.com", is the analysis done locally in CI or remotely via FOSSA?

@yurishkuro remotely.

@idvoretskyi
Copy link
Contributor Author

@yurishkuro great, so it failed now due to the licensing issues found by FOSSA, not because of the wrong configuration.

@yurishkuro
Copy link
Member

@idvoretskyi a couple questions:

  1. is there a way to tweak the GH action so that it posts the link to the report? For example, when you click on Details of the Codecov checks it takes you to their code coverage report, but FOSAA details take you to GH action log and you have to hunt for the URL of the report. Ideally it would be linked directly from Details, or even better posted as a summary as comment, like FOSSA scanning enabled for the repo #2347 (comment)
  2. The link from the logs shows NotFound, why is that? If we run this scan on all PRs, then anyone should be able to view the reports.

https://app.fossa.com/projects/custom+162%2Fgithub.com%2Fjaegertracing%2Fjaeger/refs/branch/HEAD/da0020c14818c479a6272614e192674a7ad48993

image

@idvoretskyi
Copy link
Contributor Author

@yurishkuro

  1. I'll check this and get back to you;
  2. It's the default setting, let me take a look at how can it be updated.

@idvoretskyi
Copy link
Contributor Author

@yurishkuro I've updated the FOSSA configuration here, and PR won't be blocked by the code scanning issues - they will be handled separately and should not block the PRs.

Regarding the report access - access is limited to the FOSSA UI users only, I'll go ahead and invite you (and other project maintainers) so you could have access to them.

@idvoretskyi
Copy link
Contributor Author

@yurishkuro anything else is missing to merge the PR?

@yurishkuro
Copy link
Member

I am still getting Not Found when I go to the report link shown in the logs: https://app.fossa.com/projects/custom+162%2Fgithub.com%2Fjaegertracing%2Fjaeger/refs/branch/HEAD/80a27b16e68b355ee80308f8f36e6f3ff1391541 (even after I log in to Fossa).

@idvoretskyi
Copy link
Contributor Author

@yurishkuro I don't think you've joined the CNCF FOSSA org, this is an issue.

I'll DM you with further details.

@idvoretskyi
Copy link
Contributor Author

@yurishkuro anything else is needed to get this merged?

@yurishkuro
Copy link
Member

@idvoretskyi I am confused about how this GH action is supposed to work. Before I rebased this just now, the Fossa step was showing green while the report that it was pointing to was showing 2 license issues. Shouldn't the action be shown as failed in that case?

@idvoretskyi
Copy link
Contributor Author

@yurishkuro no, we want this action to just collect reports for now. There's no need to block PRs by FOSSA reports.

@yurishkuro
Copy link
Member

Whether to block PR or not is a setting in our repo, we can make FOSSA checks non-mandatory. But for maintainers it would be good to see the actual outcome of the check when approving PR. Right now doing that is pretty involved - not only the check comes back green, but also the report itself is quite hard to get to, you have to fish out the link from one of the step logs in the GH action. As I mentioned previously, it would be much better if

  1. the check would go red if there are license violations
  2. the link next to the check would go to a report directly, OR there would be a comment posted back into PR with a brief summary, similar to code coverage FOSSA scanning enabled for the repo #2347 (comment)

Having said that, an always green check is not going to hurt anything, so I am not opposed to merging this.

@objectiser @jpkrohling @pavolloffay any thoughts?

@objectiser
Copy link
Contributor

+1 sounds ok to merge, but create an issue to make sure the actions you have suggested are investigated.

@yurishkuro yurishkuro mentioned this pull request Aug 29, 2020
2 tasks
@yurishkuro yurishkuro merged commit af985ae into jaegertracing:master Aug 29, 2020
@idvoretskyi
Copy link
Contributor Author

Umbrella issue (for the record) - cncf/foundation#109

@idvoretskyi idvoretskyi deleted the idvoretskyi-fossa-01 branch September 1, 2020 14:39
@jpkrohling
Copy link
Contributor

Can we set a target date for us to decide to keep or remove it? Having it always green even on license violations might mean that we just won't care about it at all in the future... Suggestion: by v1.21.0, we decide if we want to keep it.

@yurishkuro
Copy link
Member

Technically, we've been in this situation for several years already - the scans were running somewhere in the background, and the badge in the README kind-of reflects them, but we didn't pay attention (because it was always so noisy). The ticket #2432 lists the additional improvements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix FOSSA for all repositories
4 participants