Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

Fix solana-install init making unnecessary API requests #33949

Conversation

acheroncrypto
Copy link
Contributor

@acheroncrypto acheroncrypto commented Nov 5, 2023

Problem

Using solana-install init makes API requests to GitHub even when the given version is already installed locally.

Summary of Changes

Only fetch releases from GitHub if the given version is not installed.

Fixes #33948

@mergify mergify bot added community Community contribution need:merge-assist labels Nov 5, 2023
@mergify mergify bot requested a review from a team November 5, 2023 17:20
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Thanks for taking a crack at this!
This isn't quite right yet, as it breaks some of the other commands (solana-install info, solana-install update) that ultimately call into fn init_or_update().

Before:

$ solana-install init v1.16.13
  ✨ 1.16.13 initialized
$ solana-install info
Configuration: /.config/solana/install/config.yml
Active release directory: /.local/share/solana/install/active_release
• Release version: 1.16.13
• Release URL: https://github.com/solana-labs/solana/releases/download/v1.16.13/solana-release-aarch64-apple-darwin.tar.bz2
• Release commit: b2a38f6
  🎁 Update available: 1.16.18

With this patch:

$ cargo run --bin solana-install init v1.16.13
    Finished dev [unoptimized + debuginfo] target(s) in 0.22s
     Running `solana/target/debug/solana-install init v1.16.13`
  ✨ 1.16.13 initialized
$ cargo run --bin solana-install info
    Finished dev [unoptimized + debuginfo] target(s) in 0.22s
     Running `solana/target/debug/solana-install info`
Configuration: /.config/solana/install/config.yml
Active release directory: /.local/share/solana/install/active_release
• Release version: 1.16.13
• Release URL: https://github.com/solana-labs/solana/releases/download/v1.16.13/solana-release-aarch64-apple-darwin.tar.bz2
• Release commit: b2a38f6
  🎁 Update available: 1.16.13

I think that we need some flag to ensure your github-check bypass is only executed when the call comes from fn init(). There are probably alternatives that involve refactoring fn init_or_update(), which is kind of a kitchen sink.

@acheroncrypto
Copy link
Contributor Author

I think that we need some flag to ensure your github-check bypass is only executed when the call comes from fn init(). There are probably alternatives that involve refactoring fn init_or_update(), which is kind of a kitchen sink.

I added is_init check(already existed in the arguments) so this change should only impact the init command now. I wanted to avoid refactoring for this small change since it would be harder to review and I also didn't want to potentially break something(failed 😅).

I didn't test the other commands, sorry for that, and thanks for pointing out some of them broke.

@CriesofCarrots CriesofCarrots added the CI Pull Request is ready to enter CI label Nov 6, 2023
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Nov 6, 2023
@CriesofCarrots
Copy link
Contributor

already existed in the arguments

Oh cool, I forgot we had that already 😅

I just kicked off CI on this, so keep an eye out in case there are any complaints. Looking good, though.

Copy link

codecov bot commented Nov 6, 2023

Codecov Report

Merging #33949 (e611e06) into master (662ac8b) will decrease coverage by 0.1%.
Report is 11 commits behind head on master.
The diff coverage is 0.0%.

@@            Coverage Diff            @@
##           master   #33949     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         809      809             
  Lines      219070   219074      +4     
=========================================
- Hits       179476   179466     -10     
- Misses      39594    39608     +14     

Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution, @acheroncrypto !

@CriesofCarrots CriesofCarrots merged commit ec0ddc9 into solana-labs:master Nov 7, 2023
19 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community Community contribution need:merge-assist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unnecessary API requests to GitHub in solana-install init command
3 participants