tests: add repository maturity test #5492
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ccoVeille
reviewed
Nov 27, 2024
Comment on lines
+58
to
+59
| http.DefaultClient.Timeout = time.Minute | ||
| httpRes, err := http.DefaultClient.Do(request) |
There was a problem hiding this comment.
I would have created a dedicated client, and not playing with the default one
|
|
||
| request.Header.Set("Accept", "application/vnd.github+json") | ||
| request.Header.Set("X-GitHub-Api-Version", "2022-11-28") | ||
| request.Header.Set("User-Agent", "avelino") |
There was a problem hiding this comment.
The user agent could be a env variable too. And you should have a default one, one that respect the UserAgent format
| ) | ||
|
|
||
| var ( | ||
| githubApiAuthorizationToken = os.Getenv("GITHUB_API_TOKEN") |
There was a problem hiding this comment.
Suggested change
| githubApiAuthorizationToken = os.Getenv("GITHUB_API_TOKEN") | |
| githubAPIAuthorizationToken = os.Getenv("GITHUB_API_TOKEN") |
Or to respect the GitHub syntax
Suggested change
| githubApiAuthorizationToken = os.Getenv("GITHUB_API_TOKEN") | |
| apiTokenGitHub = os.Getenv("GITHUB_API_TOKEN") |
| t.Fatalf("failed to extract repo and user from: %s, got [%v]", href, strings.Join(matches, ", ")) | ||
| } | ||
|
|
||
| if err := checkRepositoryMaturity(matches[1], matches[2]); err != nil { |
There was a problem hiding this comment.
Ypu could use meaningful variable
Suggested change
| if err := checkRepositoryMaturity(matches[1], matches[2]); err != nil { | |
| user, repo := matches[1], matches[2] | |
| if err := checkRepositoryMaturity(user, repo); err != nil { |
| http.DefaultClient.Timeout = time.Minute | ||
| httpRes, err := http.DefaultClient.Do(request) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to fetch commits for [%s/%s], %v", user, repo, err) |
There was a problem hiding this comment.
When something is repeated like %s/%s + user, repo in almost every error you return, plus the fact the user and repository are the argument of the function, I tend to do not add them in the error message. The caller should do it
Alexizx00789999
approved these changes
Dec 6, 2024
ccoVeille
suggested changes
Dec 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was changed ?
Added a new test to check repository maturity
Why do we need it?
We need to check that projects listed here are seriously maintained. As we discussed at: #5473
How it works
When you run the test, it reads all links from the entire README file, and for each link, uses the github api to fetch the first commit made 5 months before the current date. If no commits were found, the test fails.
Only links to github repositories are checked.
How to run test?
Why do we need an API key?
Rate Limit: authenticated requests can send more requests per hour, considering that this list holds +1000 links. Not using and API key isn't an option.This PR only adds the code needed to run the test, when and how we should run them still needs a discussion, as stated above, we run tests against the entire link set. I don't think we must run this test on new PR because it may fail just because of rate limiting.
I wonder if there is a way to run them only against links added in the PR.