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

Add GitHub Action declaration for installing cargo-binstall #1269

Merged
merged 1 commit into from
Aug 9, 2023

Conversation

smallstepman
Copy link
Contributor

I'd like to suggest adding action.yml, which makes it easier for users to install cargo-binstall in GitHub Action workflows, for example:

    steps:
      - uses: cargo-bins/cargo-binstall@main
      - run: cargo binstall -y ripgrep

see complete example here and the result of running that workflow.

I'm aware of https://github.com/taiki-e/install-action, but I'd prefer to:

  • install directly from the source
  • rely on organizations rather than individual developers
  • to not have to conduct a security review of GitHub Actions that contains a few thousand lines of code and config files
git clone https://github.com/taiki-e/install-action && cd install-action/
~/install-action maincloc
───────────────────────────────────────────────────────────────────────────────
Language                 Files     Lines   Blanks  Comments     Code Complexity
───────────────────────────────────────────────────────────────────────────────
JSON                        53     14136        0         0    14136          0
Shell                        6      1289       67       131     1091        232
TOML                         4        23        2         2       19          0
YAML                         4       244       15        12      217          0
Markdown                     3      1462      552         0      910          0
Plain Text                   3       201        6         0      195          0
License                      1        23        2         0       21          0
Rust                         1       773       40        33      700        108
gitignore                    1         7        1         3        3          0
───────────────────────────────────────────────────────────────────────────────
Total                       76     18158      685       181    17292        340

@NobodyXu NobodyXu requested a review from passcod August 9, 2023 10:45
@NobodyXu
Copy link
Member

NobodyXu commented Aug 9, 2023

P.S. I prefer "rebase against main" instead of "merging from main into this PR", but since we are doing squash merge it won't matter anyway.

If we are doing merge by commit, then the commit history would be a bit confusing.

README.md Show resolved Hide resolved
.github/workflows/gh-action.yml Outdated Show resolved Hide resolved
@smallstepman smallstepman force-pushed the main branch 3 times, most recently from a904323 to 5a196e7 Compare August 9, 2023 12:28
@smallstepman smallstepman requested a review from NobodyXu August 9, 2023 12:29
.github/workflows/gh-action.yml Outdated Show resolved Hide resolved
.github/workflows/gh-action.yml Outdated Show resolved Hide resolved
@NobodyXu NobodyXu enabled auto-merge August 9, 2023 12:55
Copy link
Member

@passcod passcod left a comment

Choose a reason for hiding this comment

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

lgtm generally, just a few minor bits

.github/workflows/gh-action.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@NobodyXu NobodyXu disabled auto-merge August 9, 2023 13:09
@NobodyXu
Copy link
Member

NobodyXu commented Aug 9, 2023

CI failed due to 429 too many requests, once you update the PR I will approve and run the CI for you.

@NobodyXu NobodyXu enabled auto-merge August 9, 2023 13:39
@smallstepman
Copy link
Contributor Author

smallstepman commented Aug 9, 2023

It looks like it hung again. If the error is coming from binstalk-downloader then you might want to consider using backoff crate to handle retry timings, for example:

    let retry_policy = ExponentialBackoffBuilder::new()
        .with_initial_interval(Duration::from_secs(1))
        .with_max_interval(Duration::from_secs(16))
        .with_multiplier(2.0)
        .with_max_elapsed_time(Some(Duration::from_secs(300)))
        .build();

    let response = retry(retry_policy, || async {
        match reqwest::get(&url).await {
            Ok(response) => Ok(response),
            Err(err) => Err(backoff::Error::transient(err)),
        }
    })
    .await
    .unwrap();

@NobodyXu NobodyXu disabled auto-merge August 9, 2023 20:17
@NobodyXu NobodyXu enabled auto-merge August 9, 2023 20:17
@NobodyXu
Copy link
Member

NobodyXu commented Aug 9, 2023

It looks like it hung again. If the error is coming from binstalk-downloader then you might want to consider using backoff crate to handle retry timings, for example:

    let retry_policy = ExponentialBackoffBuilder::new()
        .with_initial_interval(Duration::from_secs(1))
        .with_max_interval(Duration::from_secs(16))
        .with_multiplier(2.0)
        .with_max_elapsed_time(Some(Duration::from_secs(300)))
        .build();

    let response = retry(retry_policy, || async {
        match reqwest::get(&url).await {
            Ok(response) => Ok(response),
            Err(err) => Err(backoff::Error::transient(err)),
        }
    })
    .await
    .unwrap();

Thanks, but we already uses backoff there, I suspect it's because we submit too many PRs tonight.

@NobodyXu NobodyXu added this pull request to the merge queue Aug 9, 2023
Merged via the queue into cargo-bins:main with commit 42dddd9 Aug 9, 2023
@smallstepman
Copy link
Contributor Author

A good problem have

@NobodyXu
Copy link
Member

Thank you @smallstepman !

@NobodyXu
Copy link
Member

A good problem have

Hmmm, I think you have just sent an incomplete comment.

@smallstepman
Copy link
Contributor Author

A good problem have

Hmmm, I think you have just sent an incomplete comment.

I was joking, I was referring to

I suspect it's because we submit too many PRs tonight

@NobodyXu
Copy link
Member

Aha you mean "A good problem to have", now I get the joke

@smallstepman
Copy link
Contributor Author

ahhh, indeed I lost a few letters ^^

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.

3 participants