-
Notifications
You must be signed in to change notification settings - Fork 173
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 fossa report
#66
Add fossa report
#66
Conversation
cmd/fossa/report.go
Outdated
if err != nil { | ||
reportLogger.Fatalf("Invalid FOSSA endpoint: %s", err.Error()) | ||
} | ||
api, err := server.Parse(fmt.Sprintf("/api/revisions/%s/dependencies", url.PathEscape(config.MakeLocator(conf.Project, conf.Revision)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is MakeLocator
in the config
package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Legacy. I'll refactor it out later, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It kinda makes sense as a config util, to take config params and make it into a certain form.
@@ -125,6 +125,18 @@ func main() { | |||
cli.BoolFlag{Name: "debug", Usage: debugUsage}, | |||
}, | |||
}, | |||
{ | |||
Name: "report", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should allow for multiple report formats.
fossa report dependencies
(default), fossa report licenses
, fossa report vulns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea. I was thinking of report types as different kinds of notices to generate (e.g. NOTICE, ATTRIBUTION, LICENSE) but this makes a lot more sense. I'll implement this.
cmd/fossa/report.go
Outdated
} | ||
|
||
type licenseResponse struct { | ||
ID string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should include SPDX id; we should prefer using SPDX in general
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix.
- `ATTRIBUTION`: generate an `ATTRIBUTION` file for your dependencies | ||
|
||
##### `-j, --json` | ||
<!-- ##### `-j, --json` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove report type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was originally thinking of report types in the context of licenses i.e. generating attribution vs. notice files. I couldn't figure out a good way to automatically generate different types of license reports, so I removed this.
This is going to be added back in to differentiate between e.g. dependency vs. license reports.
} | ||
} | ||
fmt.Println() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A thought -- in the future maybe we could fallback to querying package registries if we're missing license data.
cmd/fossa/report.go
Outdated
if err != nil { | ||
reportLogger.Fatalf("Invalid FOSSA endpoint: %s", err.Error()) | ||
} | ||
api, err := server.Parse(fmt.Sprintf("/api/revisions/%s/dependencies", url.PathEscape(config.MakeLocator(conf.Project, conf.Revision)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This depends on fossa upload
succeeding as well as all deep deps to be resolved. I would imagine the implementation would query our /api/revisions
and /api/licenses
api.
I prefer for the report
command to be independent from if the user was integrated with FOSSA; and we can offer "better" data if you used a FOSSA account.
In cases where we hit stuff that hasn't been scanned, we could rely on the fetcher.getMetadata()
responses which we'd need to build support for in FOSSA.
If we're running with this path as our v1 implementation, I would:
- File an issue proposing the fossa-independent report design
- Give user feedback that you need
fossa upload
to succeed for this command to succeed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will implement.
d6872f2
to
648dc06
Compare
PTAL. |
Usage: "Generates a license report", | ||
Action: reportCmd, | ||
Flags: []cli.Flag{ | ||
cli.StringFlag{Name: "c, config", Usage: configUsage}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fetcher?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. We're going to switch these to use global flags.
As per offline discussion, I'm going to merge this in and override merge checks. |
2455b51
to
2151729
Compare
2151729
to
835c014
Compare
This PR adds basic NOTICE generation. Reference links: