-
Notifications
You must be signed in to change notification settings - Fork 585
✨ Add multi-repo scanning: --repos, --org #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
✨ Add multi-repo scanning: --repos, --org #4793
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4793 +/- ##
==========================================
+ Coverage 66.80% 69.47% +2.67%
==========================================
Files 230 250 +20
Lines 16602 15589 -1013
==========================================
- Hits 11091 10831 -260
+ Misses 4808 3891 -917
- Partials 703 867 +164 🚀 New features to boost your workflow:
|
spencerschrock
left a comment
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 |
spencerschrock
left a comment
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.
|
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: 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. |
Signed-off-by: Spencer Schrock <[email protected]>
spencerschrock
left a comment
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.
In the interest of getting this merged, I cut out the --format=combined logic, which can be added in a follow-up when the author has time by reverting my removal commit and addressing followups.
My approval is good for all of the code written by the author, but I'll request @AdamKorcz to review my removal of the combined format code:
a22d201
justaugustus
left a comment
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.
Approving for a22d201.
ref: #4793 (review)
|
Thanks Stephen, and of course thanks Gabriel for the multi repo logic! |
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).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.
Implementation details
buildRepoURLs(ctx, *options.Options) ([]string, error):Examples
Scan a list
Scan all non-archived repos in an org
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.)