Open mattwynne opened 1 year ago
(Forgive the drive-by comment, I may be missing important context here!)
I think every GitHub API endpoint that returns a list of results paginates, usually 30 results at a time: https://docs.github.com/en/rest/overview/resources-in-the-rest-api?apiVersion=2022-11-28#pagination
The relevant call here should likely also be wrapped in octokit.paginate()
, like in the linked commit from a previous issue. It may also be worth scanning the code for other API calls that are unknowingly paginated.
I will look into implementing @cvennevik suggestion (remember that I'm allergic to TypeScript)
Also I think I like this description better than #77 but I will add @mattwynne suggestion to use the GrapghQL API:
query {
organization(login: "<YOUR_ORGANIZATION>") {
repositories(first: 100) {
edges {
node {
name
owner {
login
}
defaultBranchRef {
target {
... on Commit {
history(since: "<YOUR_DATE>", author: { email: "<YOUR_USERNAME>@users.noreply.github.com" }) {
totalCount
edges {
node {
oid
message
author {
name
email
date
}
}
}
}
}
}
}
}
}
}
}
}
@mattwynne , what I would like to do next is describe the behaviour in a .feature
file, like
the remove-inactive
To Ensure that all the members get updated.
or words to that effect. It seems that we have mostly happy path
scenarios.
The current feature description is missing that condition.
Would it be something we add to the section on changing the user status? https://github.com/cucumber/action-retire-inactive-contributors/blob/main/features/move_inactive_users.feature#L28
... or is it just something that we take care of in the corresponding testing code, similar to https://github.com/cucumber/action-retire-inactive-contributors/commit/7a4cd2e79a91b2928c1eba561fb1e8de057dbae4 ?
Honestly, something like this I would not specify in an acceptance test. It feels more like a detail of how the GitHub adapter works. I would probably just treat it as a refactoring and not bother even test-driving it. We have tests that will fail if the repo fetch stops working altogether, so that provides some protection for the refactoring.
I have implemented some changes in this PR. The OctokitGithub object will use pagination API instead to avoid missing data. But the issue is that I currently don't have a good way to write test for it. Here are my thoughts:
must be greater than 30
?I'm wondering for unit tests, should we use a mock object (like FakeGitHub
object which we already have) so that we can mock the data and test everything real quick?, and let the features
tests take care of the real implementation behavior. But then I feel like the unit tests for OctokitGithub
behavior are somewhat meaningless because it will just run exactly as we mocked... :thinking:
So I ended up with writing a simple unit test that only check for existence of an item returned from pagination API.
I really appreciate your opinions on this. As I've seen this kind of situation in my workplace, and we didn't really have anyone to discuss and resolve it.
nice work @nhv96! I left one little suggestion but I think your changes are great. Really nice tidy code, and tidying up the Error types is appreciated.
👓 What did you see?
When we tested the action here we saw that people like Aslak, who have definitely committed recently, were being retired. It looks as though we're only iterating over a subset of all the cucumber org's repos, so maybe this call is also being paginated, like the
getMembersOf
call was, until 7a4cd2e79a91b2928c1eba561fb1e8de057dbae4.✅ What did you expect to see?
Log all the cucumber org's repos, not just the first page.
This text was originally generated from a template, then edited by hand. You can modify the template here.