-
Notifications
You must be signed in to change notification settings - Fork 66
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
feat: use repo search to select repositories #397
Conversation
Using the `--repo-search` option, it is possible to supply a GitHub repository search query that is used to generate the set of repositories to process.
@lindell I've managed to get some tests on the go, so I've taken this out of draft. Very open to feedback and changes of direction, let me know what you think! |
Nice! Will try to take a look this evening :D |
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 for the contribution!
It looks very good. The biggest concern I have is accidentally writing a query that returns too many results. Which will hit the incomplete results error, and give the user an error that they don't know what to do with. Do you have any ideas about it?
PS. I fixed the linting timeout. If you merge master, it should fix the error. |
This is the default behaviour of the repo search API.
Fair point, and to be honest one that I hadn't considered enough. The doco states that:
We can handle this by either:
In my opinion, the warning will likely get lost, and the user will get unexpected results instead. I'm going to go ahead and refine the error message (option 1), but I'm happy to change this if you would like (2) or you have a different approach. |
Now suggests possible remediation for the user, and includes a test for this case.
I've updated it now so the error message reads:
Let me know what you think. |
The test was flakey because the times were all the same and map ordering is non-deterministic. Changing the time slightly for each ensures that they're going to come out in order.
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 the new error for incomplete results is good 👍
When playing around with it a bit further, for the query "org:google", where the total repos returned is 2569. It did not error on incomplete results, but instead
GET https://api.github.com/search/repositories?page=11&per_page=100&q=fork%3Atrue+org%3Agoogle: 422 Only the first 1000 search results are available
This message came after making 10 requests.
So we should also add a check if the total results are >1000 and error right away in that case.
Wraps up nil check, easier to read.
8a023ac
to
c8d13c4
Compare
The search will eventually fail, so short circuit the waiting for the user.
Fantastic feedback, much appreciated. Thanks for testing out the upper bound like that, it's much better to get a fast failure. |
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.
LGTM 👍 Thanks for the contribution!
Included in release v0.48.0 🎉 |
What does this change
Using the
--repo-search
option, it is possible to supply a GitHub repository search query that is used to generate the set of repositories to process.This feature is GItHub-only at the moment.
This allows for searches that reference:
owner:buildkite-plugins in:name docker AND plugin
owner:cultureamp owner:jamestelfer fork:true in:name plugin
I initially started this as a "warm up" to implementing something similar with code search, but the partial name search and license search are potentially quite useful in themselves.
Obviously this means that search syntax and results depend on the underlying source being queried, so implementations for GitLab et al would be required to see this feature properly take root.
References:
What issue does it fix
Closes #398
Refer also to original discussion.
Notes for the reviewer
Happy for feedback/changes of direction.
Checklist