Skip to content
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

Sort GitHub releases #3571

Merged
merged 1 commit into from Apr 24, 2022
Merged

Sort GitHub releases #3571

merged 1 commit into from Apr 24, 2022

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Apr 23, 2022

Problems

For several months now the bot has been submitting bogus auto-epoch pull requests, about 0.7 times per day:

https://github.com/KSP-CKAN/CKAN-meta/pulls?q=is%3Apr+author%3Anetkan-bot+is%3Aunmerged+is%3Aclosed

This seems to happen completely randomly, only for GitHub krefs, as if the most recent release for that mod has disappeared. However, when we check the repo, it is still there. So somehow the wrong release is being inflated.

Cause (guesses)

We don't know for sure why this happens (it's very hard to investigate something that only happens once out of 297 unique unfrozen GitHub krefs * 2 inflations per hour * 24 hours per day / 0.7 per day = 20366 attempts), but I have two main guesses at this point:

  1. The GitHub API somehow glitches out and skips the most recent release completely, so the most recent release appears to be the one before that
  2. The GitHub API returns all the releases, but it somehow glitches out and returns the releases out of order, so the most recent release is included, but it's listed further back in the list than it should be

To further the investigation of this problem, I will be attempting to rule out cause 2 here.

Changes

Now when we receive a group of releases from the API, before we choose which ones to inflate, we sort them by the published_at property, in descending order. If the API has returned the releases out of order, this will ensure that we will identify the most recent one correctly.

If the problem stops after this, then that confirms cause 2 as the culprit. If it continues, then cause 1 looks likely.

In addition, the interpretation of the JToken objects returned from the API for releases is moved from GithubApi to a new GithubRelease constructor, which allows GithubApi to deal only in properties of a release object rather than having to cast JSON tokens, etc.

@HebaruSan HebaruSan added Bug Easy This is easy to fix Pull request Netkan Issues affecting the netkan data labels Apr 23, 2022
@DasSkelett
Copy link
Member

DasSkelett commented Apr 23, 2022

We recently had this error in #netkan-bot, for a mod with only one release. The "missing required field download" also happens when there is no (full) release for a mod. This makes me think that guess 1 is the correct one, i.e. the release is "hidden" completely.
screenshot from Discord channel #netkan-bot showing error message

That said, your changes are still worth a try and sorting the releases by publish date makes sense even without this problem.

@HebaruSan
Copy link
Member Author

We recently had this error in #netkan-bot, for a mod with only one release. The "missing required field download" also happens when there is no (full) release for a mod. This makes me think that guess 1 is the correct one, i.e. the release is "hidden" completely.

Oof, good point, I didn't make that connection when that error came up. My only idea for cause 1 so far, since we can't prevent it or detect it, is to switch to the GraphQL API in the hopes that the bug is specific to the old API. Hopefully it doesn't come to that. 🤞

@HebaruSan

This comment was marked as resolved.

@HebaruSan

This comment was marked as resolved.

Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's give it a try 🤞

@HebaruSan HebaruSan merged commit d724a0f into KSP-CKAN:master Apr 24, 2022
@HebaruSan HebaruSan deleted the fix/sort-gh-rels branch April 24, 2022 00:43
@HebaruSan
Copy link
Member Author

If it continues, then cause 1 looks likely.

Cause 1 confirmed: KSP-CKAN/CKAN-meta#2686

@HebaruSan
Copy link
Member Author

HebaruSan commented Apr 27, 2022

My only idea for cause 1 so far, since we can't prevent it or detect it, is to switch to the GraphQL API in the hopes that the bug is specific to the old API.

If we tried this, would we be able to test it before going live? It seems like we would have to run it at least 20k times to detect it if the same bug was still present.

Hmm, the DownloadCounter already uses GraphQL to find releases. If the GraphQL API also sometimes returns an incomplete list of releases, then that should show up in the download_counts.json history as a sudden drop followed by a return to the previous level, a V-shape in the line graph.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Easy This is easy to fix Netkan Issues affecting the netkan data Pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants