-
Notifications
You must be signed in to change notification settings - Fork 18
fix: optimise GitLab commit history scraper #1374
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
Conversation
1649d23
to
8c84e4c
Compare
|
|
dc11bb5
to
eb732f4
Compare
ff1b5aa
to
7ad0ec3
Compare
I rebased the branch to the current status of the main branch |
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 pull request! I have some comments:
The delete button for the commit history doesn't clear the commit_history_scraped_at
field, leading to an incomplete commit history on the next harvest.
Furthermore, I have some comments on the code.
I didn't check the frontend code.
frontend/components/software/edit/services/PackageManagerServices.tsx
Outdated
Show resolved
Hide resolved
|
||
import java.util.UUID; | ||
|
||
public record BasicRepositoryDataWithHistory(UUID software, String url, String commitHistoryScrapedAt, CommitsPerWeek commitsPerWeek) { |
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 would prefer a specialised type for commitHistoryScrapedAt
, probably ZonedDateTime. I see this in other places as well, like GitlabScraper
. Or do you need strings to make it work with existing code? If so, this can also be changed later.
return new TreeMap<>(data); | ||
} | ||
|
||
public void setData(SortedMap<Instant, Long> data) { |
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'm against this method, as the data provided in the parameter might not be consistent with what this class does.
If you really need this (do you?), you should iterate over all entries in the data
map and add them with the respective addCommits
method.
static final Gson gson = new GsonBuilder() | ||
.enableComplexMapKeySerialization() | ||
.registerTypeAdapter(Instant.class, (JsonSerializer<Instant>) (src, typeOfSrc, context) -> new JsonPrimitive(src.getEpochSecond())) | ||
.create(); | ||
|
||
public SortedMap<Instant, Long> getData() { |
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.
Do you really need this other than for testing? If not, it should be package private (i.e. no visibility modifier), but I'm open for arguments for making this public. :)
77f6105
to
8faee73
Compare
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.
@paulastock Nice work for the first PR!
I have some suggestions concerning the frontend code. I would appreciate if you can join one of our standups to discuss these suggestions. I have my suggestion in code and could push additional commit with my suggestion if you like.
frontend/components/software/edit/services/ServiceInfoListItem.tsx
Outdated
Show resolved
Hide resolved
frontend/components/software/edit/services/ServiceInfoListItem.tsx
Outdated
Show resolved
Hide resolved
frontend/components/software/edit/services/ServiceInfoListItem.tsx
Outdated
Show resolved
Hide resolved
frontend/components/software/edit/services/apiSoftwareServices.tsx
Outdated
Show resolved
Hide resolved
frontend/components/software/edit/services/SoftwareRepoServices.tsx
Outdated
Show resolved
Hide resolved
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.
Please, double check my frontend suggestions.
7963756
to
1df7834
Compare
fix: deselecting top level category in dialog
…on ordering of endpoint
fix: sonar improvement suggestions chore: remove communties from default settings.json
If commit_history was scraped before, start scraping from the last timestamp stored in commit_history to reduce API calls.
1df7834
to
876f633
Compare
|
|
|
Closing this one due to issues with the history. Moving to #1432. |
Fixes #777
Changes proposed in this pull request:
commit_history_scraped_at
to start checking for new commitsHow to test:
docker compose down --volumes && docker compose build --parallel && docker compose up
PR Checklist:
docker-compose.yml