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

[#55] fetch exe_name from api if ommited #76

Closed
wants to merge 2 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 54 additions & 1 deletion src/sync/configure.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use crate::config::schema::ConfigAsset;
use crate::model::asset_name::AssetName;
use crate::model::tool::{Tool, ToolError, ToolInfo, ToolInfoTag};
use crate::sync::db::lookup_tool;
use serde::Deserialize;

pub fn configure_tool(tool_name: &str, config_asset: &ConfigAsset) -> Tool {
match lookup_tool(tool_name) {
Expand All @@ -27,11 +28,31 @@ pub fn configure_tool(tool_name: &str, config_asset: &ConfigAsset) -> Tool {
}
}

#[derive(Deserialize)]
pub struct RepoInformation {
pub name: String,
// There are other fields too, but we don't need them now
// They may be added later if needed at all
}

fn get_exe_name(owner: String, repo: String) -> Result<String, ureq::Error> {
let url = format!(
"https://api.github.com/repos/{owner}/{repo}",
owner = owner,
repo = repo
);
let repo: RepoInformation = ureq::get(&url).call()?.into_json()?;
Ok(repo.name)
}

/// Configure 'ToolInfo' completely from 'ConfigAsset'
fn full_configure(config_asset: &ConfigAsset) -> Option<ToolInfo> {
let owner = config_asset.owner.clone()?;
let repo = config_asset.repo.clone()?;
let exe_name = config_asset.exe_name.clone()?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this line is just

    let exe_name = config_asset.exe_name.clone().unwrap_or(config_asset.repo.clone()?);

Can we not let it error and give some type of output to the user to add an exe_name to the config?
This way there is no need for an extra api call.

Another approach would be looking for the common substring in the asset names. But I am more in favor of letting the error bubble up so the tool does not download something if the user forgets to add an exe_name or they mistake the name of the binary with the repo.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is a small piece of the json returned by the repos/{owner}/repo/release/tags/{tag} endpoint.

The name is always formatted as {name}-{tag}-{arch}.some.zip.extension


  "assets": [
    {
      "url": "https://api.github.com/repos/BurntSushi/ripgrep/releases/assets/38486868",
      "id": 38486868,
      "node_id": "MDEyOlJlbGVhc2VBc3NldDM4NDg2ODY4",
      "name": "ripgrep-13.0.0-arm-unknown-linux-gnueabihf.tar.gz",
      "label": "",
     ...
      "content_type": "application/octet-stream",
      "state": "uploaded",
      "size": 1763861,
      "download_count": 3387,
      "created_at": "2021-06-12T12:31:57Z",
      "updated_at": "2021-06-12T12:31:58Z",
      "browser_download_url": "https://github.com/BurntSushi/ripgrep/releases/download/13.0.0/ripgrep-13.0.0-arm-unknown-linux-gnueabihf.tar.gz"
    },
    {
      "url": "https://api.github.com/repos/BurntSushi/ripgrep/releases/assets/38486879",
      "id": 38486879,
      "node_id": "MDEyOlJlbGVhc2VBc3NldDM4NDg2ODc5",
      "name": "ripgrep-13.0.0-i686-pc-windows-msvc.zip",
      "label": "",
    ...
      "content_type": "application/octet-stream",
      "state": "uploaded",
      "size": 1591463,
      "download_count": 3282,
      "created_at": "2021-06-12T12:32:47Z",
      "updated_at": "2021-06-12T12:32:48Z",
      "browser_download_url": "https://github.com/BurntSushi/ripgrep/releases/download/13.0.0/ripgrep-13.0.0-i686-pc-windows-msvc.zip"
    },
    {
      "url": "https://api.github.com/repos/BurntSushi/ripgrep/releases/assets/38486907",
      "id": 38486907,
      "node_id": "MDEyOlJlbGVhc2VBc3NldDM4NDg2OTA3",
      "name": "ripgrep-13.0.0-x86_64-apple-darwin.tar.gz",
      "label": "",
   ...
      "content_type": "application/octet-stream",
      "state": "uploaded",
      "size": 1815615,
      "download_count": 192942,
      "created_at": "2021-06-12T12:35:00Z",
      "updated_at": "2021-06-12T12:35:00Z",
      "browser_download_url": "https://github.com/BurntSushi/ripgrep/releases/download/13.0.0/ripgrep-13.0.0-x86_64-apple-darwin.tar.gz"
    },
    {
      "url": "https://api.github.com/repos/BurntSushi/ripgrep/releases/assets/38486889",
      "id": 38486889,
      "node_id": "MDEyOlJlbGVhc2VBc3NldDM4NDg2ODg5",
      "name": "ripgrep-13.0.0-x86_64-pc-windows-gnu.zip",
      "label": "",
     ...
      "content_type": "application/octet-stream",
      "state": "uploaded",
      "size": 9811405,
      "download_count": 10684,
      "created_at": "2021-06-12T12:33:25Z",
      "updated_at": "2021-06-12T12:33:26Z",
      "browser_download_url": "https://github.com/BurntSushi/ripgrep/releases/download/13.0.0/ripgrep-13.0.0-x86_64-pc-windows-gnu.zip"
    },
    {
      "url": "https://api.github.com/repos/BurntSushi/ripgrep/releases/assets/38486875",
      "id": 38486875,
      "node_id": "MDEyOlJlbGVhc2VBc3NldDM4NDg2ODc1",
      "name": "ripgrep-13.0.0-x86_64-pc-windows-msvc.zip",
      "label": "",
     ...
      "content_type": "application/octet-stream",
      "state": "uploaded",
      "size": 1734357,
      "download_count": 30550,
      "created_at": "2021-06-12T12:32:30Z",
      "updated_at": "2021-06-12T12:32:30Z",
      "browser_download_url": "https://github.com/BurntSushi/ripgrep/releases/download/13.0.0/ripgrep-13.0.0-x86_64-pc-windows-msvc.zip"
    },
    {
      "url": "https://api.github.com/repos/BurntSushi/ripgrep/releases/assets/38486871",
      "id": 38486871,
      "node_id": "MDEyOlJlbGVhc2VBc3NldDM4NDg2ODcx",
      "name": "ripgrep-13.0.0-x86_64-unknown-linux-musl.tar.gz",
      "label": "",
    ...
      "content_type": "application/octet-stream",
      "state": "uploaded",
      "size": 2109801,
      "download_count": 272291,
      "created_at": "2021-06-12T12:32:02Z",
      "updated_at": "2021-06-12T12:32:03Z",
      "browser_download_url": "https://github.com/BurntSushi/ripgrep/releases/download/13.0.0/ripgrep-13.0.0-x86_64-unknown-linux-musl.tar.gz"
    },
    {
      "url": "https://api.github.com/repos/BurntSushi/ripgrep/releases/assets/38493219",
      "id": 38493219,
      "node_id": "MDEyOlJlbGVhc2VBc3NldDM4NDkzMjE5",
      "name": "ripgrep_13.0.0_amd64.deb",
      "label": null,
     ...
      "content_type": "application/vnd.debian.binary-package",
      "state": "uploaded",
      "size": 1574096,
      "download_count": 71172,
      "created_at": "2021-06-12T17:36:20Z",
      "updated_at": "2021-06-12T17:36:21Z",
      "browser_download_url": "https://github.com/BurntSushi/ripgrep/releases/download/13.0.0/ripgrep_13.0.0_amd64.deb"
    }
  ],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we not let it error and give some type of output to the user to add an exe_name to the config?

I don't have many examples in my mind where the tool name is different than repo name and needs explicit user handling, so I don't think that benefits the end user.

The only case where I think manual handling would be beneficial, might be where two or more tools have the same name, or at least same repo name, which I think is a very rare case.

Here is a small piece of the json returned by the repos/{owner}/repo/release/tags/{tag} endpoint.

The name is always formatted as {name}-{tag}-{arch}.some.zip.extension

Most of repos, especially the big ones, have a nice consistent format, but it's not forced and so, not always done consistently

An example of such would be teeldear where assets' names are in {name}-{arch}.* format:

:;curl -sL 'https://api.github.com/repos/dbrgn/tealdeer/releases' | jq '.[] | .assets | .[] | .name' | head -15
"completions_bash"
"completions_fish"
"completions_zsh"
"LICENSE-APACHE.txt"
"LICENSE-MIT.txt"
"tealdeer-linux-arm-musleabi"
"tealdeer-linux-arm-musleabi.sha256"
"tealdeer-linux-arm-musleabihf"
"tealdeer-linux-arm-musleabihf.sha256"
"tealdeer-linux-armv7-musleabihf"
"tealdeer-linux-armv7-musleabihf.sha256"
"tealdeer-linux-i686-musl"
"tealdeer-linux-i686-musl.sha256"
"tealdeer-linux-x86_64-musl"
"tealdeer-linux-x86_64-musl.sha256"

Copy link
Owner

Choose a reason for hiding this comment

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

The original issue #55 was about the ability to not specify the exe_name field when the repo field is specified. repo is a required field (unless the tool is hardcoded in tool-sync, in that case, you don't need to specify anything) so we always know the repo name. This way, we don't even need to query the GitHub API to get the name. We can just use repo from the config.

So I propose to move this discussion and the implementation away from the GitHub API-based implementation toward a simpler design 🙂

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I didnt know the release binary names aren't generated.

let exe_name = match config_asset.exe_name.clone() {
Some(exe_name) => exe_name,
None => get_exe_name(owner.clone(), repo.clone()).unwrap(),
};
let tag = config_asset
.tag
.clone()
Expand Down Expand Up @@ -219,6 +240,38 @@ mod tests {
);
}

#[test]
fn get_exe_name() {
let tool_name = "abcdef";

let config_asset = ConfigAsset {
owner: Some(String::from("chshersh")),
repo: Some(String::from("tool-sync")),
exe_name: None,
asset_name: AssetName {
linux: Some(String::from("my-linux")),
macos: Some(String::from("my-macos")),
windows: Some(String::from("yours-windows")),
},
tag: Some(String::from("1.0.0")),
};

assert_eq!(
configure_tool(tool_name, &config_asset),
Tool::Known(ToolInfo {
owner: "chshersh".to_string(),
repo: "tool-sync".to_string(),
exe_name: "tool-sync".to_string(),
asset_name: AssetName {
linux: Some("my-linux".to_string()),
macos: Some("my-macos".to_string()),
windows: Some("yours-windows".to_string()),
},
tag: ToolInfoTag::Specific("1.0.0".to_string()),
})
);
}

#[test]
fn partial_override() {
let tool_name = "ripgrep";
Expand Down