-
Notifications
You must be signed in to change notification settings - Fork 578
✨ Add multi-repo scanning: --repos, --org, and optional --combined output #4793
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4793 +/- ##
==========================================
+ Coverage 66.80% 67.48% +0.67%
==========================================
Files 230 250 +20
Lines 16602 19395 +2793
==========================================
+ Hits 11091 13088 +1997
- Misses 4808 5428 +620
- Partials 703 879 +176 🚀 New features to boost your workflow:
|
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.
Did a partial review of a few things before I ran out of time, I need to dive more into how you build the repo list, scan repos, and present the results. Feel free to address that feedback now or wait until I have more time for full review
Also, this repo requires DCO. Lines 10 to 14 in 2864c76
For instructions on how to fix it: |
3ab90d2
to
75c4984
Compare
Just wanted to update you that this is still on my radar! We are trying to cut a 5.3.0 release this week, so my attention has been on that. But once that's cut this week, I will make time to finish. my review, and happy to cut a 5.4.0 or 5.3.1 shortly after |
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 like this approach overall, I think building a slice of repos and looping over them is the correct approach. Most of my issue is with the --combined
output format, is there anyway we can split that into its own PR? I think that would help get --repos
and --org
merged first.
options/flags.go
Outdated
|
||
FlagCombinedOutput = "combined" |
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.
is this intended to be only change the behavior if --format=default
? Currently when I run something like this, I get the "combined" output instead of "json"
go run main.go --org=ossf-tests --format json --combined
Or should this be it's own format value? e.g.
go run main.go --org=ossf-tests --format combined
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 was not intended, but after reviewing, I definitely prefer your approach to treat combined as a new format option.
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 now changed, now we use --format combined
... I intentionally left the start/finish banner for the format default and combined. Maybe these banners should be optional, using another flag (--show-progress
or something like that).
if err != nil { | ||
// processRepo already logged details; skip this URI. | ||
fmt.Fprintf(os.Stderr, "Skipping %s: %v\n", uri, err) | ||
continue | ||
} |
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 changes the behavior of the scorecard
binary. Previously if any check had a runtime error, rootCmd
would return an error, which would lead to a non-zero exit code. But now these runtime errors don't lead to an exit code.
I think continuing to scan all repositories is good, but we should "remember" if we saw an error along the way so we can return it at the end
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.
Makes sense. I added a small implementation for this using sawRuntimeErr
. We can improve it, but I think it already aligns with your point.
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.
Thanks, I may change the implementation in a follow-up as it would just be more effective for me to handle my suggested changes.
cmd/root.go
Outdated
label := repoLabelFromURI(uri) | ||
|
||
// Start banners with repo label (always show banners even in combined-only mode) | ||
if o.Format == options.FormatDefault { | ||
if len(enabledProbes) > 0 { | ||
printProbeStart(label, enabledProbes) | ||
} else { | ||
printCheckStart(label, enabledChecks) | ||
} |
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.
let's get rid of repoLabelFromURI
and just use uri
instead of label
Otherwise it would be confusing when you analyze repos across forges:
--repos=github.com/some/repo,gitlab.com/other/repo
Which one is github? Which one is gitlab? What if they both have the exact same repo name?
Starting (some/repo) [Maintained]
...
Starting (other/repo) [Maintained]
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 point. And much easier. Changed.
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.
The only cosmetic thing is that running with --org, we will get as uri the full URL, like https://github.com/ossf-tests/scorecard-check-pinned-dependencies-e2e
...
This pull request has been marked stale because it has been open for 10 days with no activity |
Hey @spencerschrock, thank you so much for reviewing my PR — and sorry it took me a while to address your comments; I couldn’t get to it earlier. I left two small cosmetic notes (one about the banners and one about the URI) for us to discuss. Whenever you have a moment, this is ready for another review. ✌️ |
Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
Signed-off-by: Jessica Wagantall <[email protected]> Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
Signed-off-by: Stephen Augustus <[email protected]> Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
Fix typo in message about pinning dependencies. Signed-off-by: Martin Costello <[email protected]> Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
Remove redundant code from when `pull_request_target` was used that was removed by ossf#2672. Signed-off-by: Martin Costello <[email protected]> Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
Signed-off-by: Mike Dolan <[email protected]> Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
…4814) Bumps the distroless group with 1 update in the /clients/githubrepo/roundtripper/tokens/server directory: distroless/base. Bumps the distroless group with 1 update in the /cron/internal/bq directory: distroless/base. Bumps the distroless group with 1 update in the /cron/internal/cii directory: distroless/base. Bumps the distroless group with 1 update in the /cron/internal/controller directory: distroless/base. Bumps the distroless group with 1 update in the /cron/internal/webhook directory: distroless/base. Bumps the distroless group with 1 update in the /cron/internal/worker directory: distroless/base. Updates `distroless/base` from `06c713c` to `10136f3` Updates `distroless/base` from `06c713c` to `10136f3` Updates `distroless/base` from `06c713c` to `10136f3` Updates `distroless/base` from `06c713c` to `10136f3` Updates `distroless/base` from `06c713c` to `10136f3` Updates `distroless/base` from `06c713c` to `10136f3` --- updated-dependencies: - dependency-name: distroless/base dependency-version: nonroot dependency-type: direct:production dependency-group: distroless - dependency-name: distroless/base dependency-version: nonroot dependency-type: direct:production dependency-group: distroless - dependency-name: distroless/base dependency-version: nonroot dependency-type: direct:production dependency-group: distroless - dependency-name: distroless/base dependency-version: nonroot dependency-type: direct:production dependency-group: distroless - dependency-name: distroless/base dependency-version: nonroot dependency-type: direct:production dependency-group: distroless - dependency-name: distroless/base dependency-version: nonroot dependency-type: direct:production dependency-group: distroless ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
…4819) * repair GitHub project list with excess path components Approximately 300 repos in the list had 1-3 extra path components, which caused 32 shards to continuously have runtime errors: Could not resolve to a Repository with the name '/' They were fixed by dropping all excess path components using a VSCode find-and-replace regex, then `make add-projects` to remove duplicates: find: github\.com/([^/]+)/([^/]+)/[^/]+ replace: github.com/$1/$2, Signed-off-by: Spencer Schrock <[email protected]> * remove 404 and add renamed repos This was done with a quick one-off script to hit the "get a repository" endpoint to test the new repos: https://api.github.com/repos/OWNER/REPO Signed-off-by: Spencer Schrock <[email protected]> --------- Signed-off-by: Spencer Schrock <[email protected]> Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
91600a2
to
07e8adc
Compare
Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
Signed-off-by: Gabriel Alejandro Soltz <[email protected]>
I will get to this tomorrow, thanks for your patience. |
// End banners BEFORE RESULTS (always show banners even in combined-only mode) | ||
if o.Format == options.FormatDefault { | ||
if len(enabledProbes) > 0 { | ||
printProbeResults(uri, enabledProbes) | ||
} else { | ||
printCheckResults(uri, enabledChecks) |
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.
as written, this only prints the results for the default format, but the comment makes me think it should also apply to the combined format:
if o.Format == options.FormatDefault || o.Format == options.FormatCombined {
if err != nil { | ||
// processRepo already logged details; skip this URI. | ||
fmt.Fprintf(os.Stderr, "Skipping %s: %v\n", uri, err) | ||
continue | ||
} |
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.
Thanks, I may change the implementation in a follow-up as it would just be more effective for me to handle my suggested changes.
CommitDepth int | ||
ShowDetails bool | ||
ShowAnnotations bool | ||
CombinedOutput bool |
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.
with the switch to using the existing format flag, this CombinedOutput
field is unused
// FormatCombined specifies that results should be output as a single combined table for multiple repos. | ||
FormatCombined = "combined" |
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.
Can we imply this is just for the table output since the other formats naturally combine their output?
FormatCombinedTable = "combined-table"
// FormatCombinedResults prints a combined table with extra REPO and AGGREGATED SCORE columns. | ||
// This expects `results` to contain checks for a single repo; callers scanning | ||
// multiple repos should aggregate calls or invoke this helper appropriately. | ||
func FormatCombinedResults( | ||
writer io.Writer, | ||
results *Result, | ||
checkDocs docChecks.Doc, | ||
opt *AsStringResultOption, | ||
opts *options.Options, | ||
) error { |
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.
one of the reasons I suggested splitting this into a separate PR was to get the other code merged, since I knew I would have trickier feedback on this format that would take another cycle or two.
- All the other format options are methods on results. Is there a reason you didn't go with that approach instead of
FormatCombinedResults
:
func (r *Result) AsCombinedTable(writer io.Writer, checkDocs docChecks.Doc, opt *AsStringResultOption) error
- There's a lot of code overlap with
AsString
, the main difference is the two additional columns. Instead of duplicating the logic, can you make them use a shared helper function with a flag or something?
e.g.
func (r *Result) AsString(writer io.Writer, checkDocs docChecks.Doc, opt *AsStringResultOption) error {
combined := false
return common(writer, checkDocs, opt, combined)
}
func (r *Result) AsCombined(writer io.Writer, checkDocs docChecks.Doc, opt *AsStringResultOption) error {
combined := true
return common(writer, checkDocs, opt, combined)
}
and then in `common` you can look at that bool to see if you should add the column or not
What kind of change does this PR introduce?
This PR introduces support for scanning multiple repositories in a single invocation, while preserving existing behavior by default. The only user-visible change for single-repo usage is a small improvement to the “Starting/Finished” banners (they now include the repo label).
New flags
--repos
: comma-separated list of repositories to scan (e.g., --repos=owner1/repo1,github.com/owner2/repo2).--org
: GitHub organization handle (e.g., --org=github.com/ossf or ossf).--combined
: after scanning N repos, print a single combined table with all checks together with two new extra columns, REPO and AGGREGATED SCORE.Precedence when multiple inputs are provided: --repos ➜ --org ➜ --local ➜ --repo (or repo resolved from package managers).
UX / output changes
Before (single repo):
Now (single or multi-repo):
This makes it obvious which repo each check belongs to—especially important when scanning many repos.
No other output changes occur unless
--combined
is used.When
--combined
is set, a final COMBINED RESULTS section is emitted after all per-repo results, e.g.:Implementation details
buildRepoURLs(ctx, *options.Options) ([]string, error)
:Examples
Scan a list
Scan all non-archived repos in an org
Aggregate across many repos
PR title follows the guidelines defined in our pull request documentation
Tests for the changes have been added (for bug fixes/features)
Which issue(s) this PR fixes
Fixes #4792
Special notes for your reviewer
Does this PR introduce a user-facing change?
For user-facing changes, please add a concise, human-readable release note to
the
release-note
(In particular, describe what changes users might need to make in their
application as a result of this pull request.)