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

use channel over github api #104

Merged
merged 57 commits into from
Aug 8, 2022
Merged

use channel over github api #104

merged 57 commits into from
Aug 8, 2022

Conversation

eightfilms
Copy link
Contributor

@eightfilms eightfilms commented Jul 15, 2022

Closes #87

Note: we might want to wait for #95 to be approved and merged prior to getting this merged

Now that we have channels we should move to using them instead of directly forming URLs within the code itself. This also gets rid of the flakiness issue of getting versions from GitHub API, which fails sometimes as reported here as well.

  • new channel.rs to handle all things Channel related. Eventually we should also support nightly and stable here. There's also a simple test included here to verify correctness of parsing a sample channel (provided within tests dir)

  • Under this new file there is also the Package construct which represent a downloadable component and its different distribution info, represented by a HashedBinary which contains the respective url and hash based on the target architecture. Currently we do not check the hash when we download the file but this can be done in a future PR.

  • Introduced new way of creating a DownloadCfg from a Package. This allows fuelup to use the download URL directly without having to manually build and format the URL.

  • code changes for existing code has to do with commands fuelup check and fuelup toolchain install latest. check has been updated to parse the versions from the channel toml, while toolchain install latest has been updated to use DownloadCfg::from_package to directly download from the url within the correct HashedBinary representation (depending on your machine)

  • There is also some refactoring of existing code (eg. get_latest_tag()), mostly to do with download.rs to make things smoother when handling both channel usage and github api usage.

  • We do not yet deprecate GitHub API support completely for at least a few versions to ensure stability, so we keep them as a fallback for now.

@eightfilms eightfilms self-assigned this Jul 15, 2022
@eightfilms eightfilms added the enhancement enhancement to a current feature label Jul 15, 2022
@eightfilms eightfilms requested review from a team, adlerjohn and mitchmindtree July 15, 2022 07:25
@eightfilms eightfilms marked this pull request as ready for review July 15, 2022 07:26
@eightfilms eightfilms marked this pull request as draft July 18, 2022 03:35
Copy link
Contributor

@mitchmindtree mitchmindtree left a comment

Choose a reason for hiding this comment

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

Getting closer! Just a few more nits.

Hmm, I actually discovered a toml parsing bug as a result of #100. I have included the fix in this PR because it was only a one-liner fix (in this commit 1ac5155)

Looking a bit closer at the Settings type, it seems to be doing some manual parsing that might also be better replaced by deriving Deserialize? Probably best addressed in a separate PR either way.

src/download.rs Outdated
@@ -298,10 +290,11 @@ pub fn unpack_bins(dir: &Path, dst_dir: &Path) -> Result<Vec<PathBuf>> {

#[cfg(test)]
mod tests {
use crate::channel::Channel;

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

src/channel.rs Outdated
@@ -3,21 +3,34 @@ use crate::{
download::DownloadCfg,
};
use anyhow::{bail, Result};

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

src/channel.rs Outdated
use tempfile::tempdir_in;
use toml_edit::{Document, Item};
use tracing::error;
use toml_edit::{de, Item};

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

src/channel.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/download.rs Outdated Show resolved Hide resolved
@eightfilms
Copy link
Contributor Author

Looking a bit closer at the Settings type, it seems to be doing some manual parsing that might also be better replaced by deriving Deserialize?

Good point - will create an issue for this

JoshuaBatty
JoshuaBatty previously approved these changes Jul 25, 2022
Copy link
Member

@JoshuaBatty JoshuaBatty left a comment

Choose a reason for hiding this comment

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

LGTM, will defer to @mitchmindtree for final approval.

* fuelup check refactor

* Better error handling when splitting and parsing versions

* clippy and message changes

* revert check logic changes

* remove newline
@eightfilms eightfilms mentioned this pull request Jul 27, 2022
4 tasks
mitchmindtree
mitchmindtree previously approved these changes Jul 28, 2022
Copy link
Contributor

@mitchmindtree mitchmindtree left a comment

Choose a reason for hiding this comment

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

🚢

@eightfilms eightfilms merged commit 557d1e9 into master Aug 8, 2022
@eightfilms eightfilms deleted the bingcicle/channel-support branch August 8, 2022 00:55
eightfilms pushed a commit that referenced this pull request Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement enhancement to a current feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor fuelup to use channels
3 participants