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

GitHub tools #1039

Draft
wants to merge 8 commits into
base: develop
Choose a base branch
from
Draft

GitHub tools #1039

wants to merge 8 commits into from

Conversation

flying-sausages
Copy link
Member

Description

Standardise use of the github api for querying for download URLs and versions

Proposed Changes:

  • New function for getting download URLs which also exports the retrieved tag as a variable

Change Categories

  • Change in functions

Checklist

  • Branch was made off the develop branch and the PR is targetting the develop branch
  • Docs have been made OR are not necessary
    • PR link:
  • Changes to panel have been made OR are not necessary
    • PR link:
  • Code is formatted (See more)
  • Code conforms to project structure (See more)
  • Shellcheck isn't screaming (See more)
  • Prints to terminal are handled (See more)
  • I have commented my code, particularly in hard-to-understand areas
  • Testing was done
    • Tests created or no new tests necessary
    • Tests executed

Test scenarios

Should test that all affected installers work as expected

Architectures

amd64 armhf arm64 Unspecified
Jammy
Focal
Bookworm
Bullseye
Buster
Raspbian ⚫️ ⚫️ ⚫️

✅❎ Passed

🛠🛠 TODO

❌❌ Currently failing

@liaralabs
Copy link
Member

Before you invest a lot of time in this, the whole point of the github latest version function currently in use is precisely that it doesn't use the github api because the rate limits can be avoided entirely this way

@flying-sausages flying-sausages changed the base branch from master to develop July 15, 2023 10:01
@flying-sausages
Copy link
Member Author

ok 60/hour is probably something that is indeed going to bite us if we use the API.

Yeah changing the guts of the function to one that uses the "scraper" could be beneficial so we get rid of the API use in the rest of the codebase easier as well, and can then just adapt our approach in one swoop in case there's a change in how the release page looks.

Could just hop onto your latest tag function and then return a formatted string for the download URL

How do you feel about exporting the tag version as a variable as a byproduct of this function? It smells like an anti-pattern to me

@flying-sausages
Copy link
Member Author

Re-wrote this without using the API, and found the endpoint Github uses to generate the list of releases that's used by their frontend so we can parse it a lot easier as well.

The code is a lot cleaner now too, but I'm still unsure about the version variable. I'm thinking we can just expect that you get the version first if you need it as a variable, and you can pass it into this function so you can skip the call as well?


pyvenv_version=$(/opt/.venv/sabnzbd/bin/python --version | awk '{print $2}')

if dpkg --compare-versions ${pyvenv_version} lt 3.8.0 && dpkg --compare-versions ${latestversion} ge 3.5.0; then
if dpkg --compare-versions ${pyvenv_version} lt 3.8.0 && dpkg --compare-versions ${github_sabnzbd_version} ge 3.5.0; then
Copy link
Member

Choose a reason for hiding this comment

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

${github_sabnzbd_version} is undefined

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants