-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Refactor contributors list & fetch wrapper [i18nIgnore] #12530
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
✅ Deploy Preview for astro-docs-2 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
I confirmed this was a token issue and updating our token fixed deploys. Nonetheless, some good action items we should look at coming out of this:
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
Updated the PR based on your recommendations:
|
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 looks fantastic @HiDeoo! +1 to the retry logic and nice work moving this to a workflow. Great to see it cleaned up.
We only store the data we really need (e.g. not the number of contributions which will make diffs a bit easier — I guess showing (and storing) contributors alphabetically rather than number of contributions would make this even easier if we wanted).
Off the top of my head, we ultimately render in order from most contributions to least, so I guess we have two options here:
- Store pre-sorted contributors — only requires storing usernames.
- Store alphabetically sorted contributors — would require storing username + contribution count.
I don’t have strong opinions on which is better. Option 1 I guess will result in a slightly messier diff as entries move up and down, while option 2 will be more stable but all contributors in a week will show up as their contribution count changes.
One thought though: we’re not using the id
field right? Why not make the login
value the id
and just use that? So with the current approach the file would look like:
[
{
"id": "jsparkdev"
},
{
"id": "ArmandPhilippot"
},
{
"id": "sarah11918"
},
// etc.
]
Or if we included a count:
[
{
"id": "ArmandPhilippot",
"count": 100
},
{
"id": "jsparkdev",
"count": 1000
},
{
"id": "sarah11918",
"count": 500
},
// etc.
]
There’s also another option, which is to not bother with content collections for this and just store:
[
"jsparkdev",
"ArmandPhilippot",
"sarah11918",
// etc.
]
The component could just import
the JSON file for this one — we don’t really need the content collection features.
Description (required)
The issue has been found and is related to a request to the GitHub API to fetch contributors returning a 403 status code.
The issue will not be fixed in this PR but I'm keeping the investigation notes here for reference and also as a reminder to refactor this part of the code to add more logging when fetching failures happen and also revisit the retry logic (the default settings exceeds the Netlify build time limit).
renderToString
) "Contribute to Astro"Bad response for https://api.github.com/repos/withastro/docs/contributors (403): Forbidden
Related issues & labels (optional)