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

godownloader refactor for allow using a GH Token, other improvements #1673

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

seanmuth
Copy link

@seanmuth seanmuth commented Jul 3, 2024

Description

  • If TAG == LATEST_VERSION assume that's correct and skip the API call entirely
  • Allow passing a GitHub Token with -t to make Authenticated calls to the GH API with a much higher rate limit
  • Capture and parse the http status code in the github_release() method to properly log the status/message
  • I also removed the conditional call to releases/latest since we never make a call to github_release() without supplying a tag/version so that condition is never met.

🧪 Functional Testing

❯ cat godownloader.sh| sudo bash -s -- -d v1.27.0
astronomer/astro-cli info checking GitHub for tag 'v1.27.0'
astronomer/astro-cli info HTTP Status Code 403: {"message":"API rate limit exceeded for <IP redacted>. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)","documentation_url":"https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting"}
astronomer/astro-cli crit unable to find 'v1.27.0' - see https://github.com/astronomer/astro-cli/releases for details

❯ cat godownloader.sh| sudo bash -s -- -d -t <redacted_token> v1.27.0
astronomer/astro-cli info checking GitHub for tag 'v1.27.0'
astronomer/astro-cli debug HTTP Status Code 200
astronomer/astro-cli info found version: 1.27.0 for v1.27.0/darwin/arm64
astronomer/astro-cli debug downloading files into /var/folders/zz/zyxvpxvq6csfxvn_n0000000000000/T/tmp.4xXlmreS
astronomer/astro-cli debug http_download https://github.com/astronomer/astro-cli/releases/download/v1.27.0/astro_1.27.0_darwin_arm64.tar.gz
astronomer/astro-cli debug http_download https://github.com/astronomer/astro-cli/releases/download/v1.27.0/astro_1.27.0_checksums.txt
astronomer/astro-cli info installed ./bin/astro

❯ cat godownloader.sh| sudo bash -s -- -d -t <redacted_token> v1.27.1
astronomer/astro-cli info Using Latest(v1.27.1) version
astronomer/astro-cli info found version: 1.27.1 for v1.27.1/darwin/arm64
astronomer/astro-cli debug downloading files into /var/folders/zz/zyxvpxvq6csfxvn_n0000000000000/T/tmp.LNNZC7Uy
astronomer/astro-cli debug http_download https://github.com/astronomer/astro-cli/releases/download/v1.27.1/astro_1.27.1_darwin_arm64.tar.gz
astronomer/astro-cli debug http_download https://github.com/astronomer/astro-cli/releases/download/v1.27.1/astro_1.27.1_checksums.txt
astronomer/astro-cli info installed ./bin/astro

❯ cat godownloader.sh| sudo bash -s -- -d -t <redacted_token> v1.26.0
astronomer/astro-cli info checking GitHub for tag 'v1.26.0'
astronomer/astro-cli debug HTTP Status Code 200
astronomer/astro-cli info found version: 1.26.0 for v1.26.0/darwin/arm64
astronomer/astro-cli debug downloading files into /var/folders/zz/zyxvpxvq6csfxvn_n0000000000000/T/tmp.rMrLeSVx
astronomer/astro-cli debug http_download https://github.com/astronomer/astro-cli/releases/download/v1.26.0/astro_1.26.0_darwin_arm64.tar.gz
astronomer/astro-cli debug http_download https://github.com/astronomer/astro-cli/releases/download/v1.26.0/astro_1.26.0_checksums.txt
astronomer/astro-cli info installed ./bin/astro

📋 Checklist

I didn't both setting up a local astro env to test against because this is only a change the the downloader script, not actually changes to the CLI code. If I still need to do this, LMK and I'll get my env setup for it.

But because this script is what's returned from curl -sSL install.astronomer.io and in all of our CLI install docs, this should be reviewed closely!

  • Rebased from the main (or release if patching) branch (before testing)
  • Ran make test before taking out of draft
  • Ran make lint before taking out of draft
  • Added/updated applicable tests
  • Tested against Astro-API (if necessary).
  • Tested against Houston-API and Astronomer (if necessary).
  • Communicated to/tagged owners of respective clients potentially impacted by these changes.
  • Updated any related documentation

- Allow passing a GitHub Token with -t to make Authenticated calls to the GH API with a much higher rate limit
- Capture and parse the http status code in the github_release() method to properly log the status/message
Copy link

codecov bot commented Jul 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.35%. Comparing base (1162f34) to head (8216e88).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1673   +/-   ##
=======================================
  Coverage   86.35%   86.35%           
=======================================
  Files         116      116           
  Lines       16977    16977           
=======================================
  Hits        14660    14660           
  Misses       1394     1394           
  Partials      923      923           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@seanmuth
Copy link
Author

seanmuth commented Jul 3, 2024

Discussed this with @ashb a bit, gonna remove the -t flag and just check if a GITHUB_TOKEN env var has been set, that way we don't need to pass tokens in the commandline, and if wouldn't require messing with the install.astronomer.io script

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

Successfully merging this pull request may close these issues.

1 participant