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

fix(ci): code coverage actions/upload-artifact version #96

Conversation

oleonardolima
Copy link
Collaborator

Description

The Code Coverage step seems to start failing due to actions/checkout@v2 becoming deprecated, and an error now instead of a warning. At least it's what I got from the error message here: https://github.com/bitcoindevkit/rust-esplora-client/actions/runs/10702593999/job/29671147653?pr=93

This PR bumps it's version to v4, the latest one. I'm also taking the opportunity to bump the action/checkout to v4 too.

I'm not sure if it became fully/enforced deprecation from yesterday to today, because it ran successfully on the last master merged PR CI steps #:thinking:

Notes to the reviewers

Please let me know if I should take the opportunity to bump any other actions, or update the workflow in any way.

Changelog notice

  • Bump actions/upload-artifact to v4.
  • Bump actions/checkout to v4.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

@ValuedMammal
Copy link
Contributor

This looks good to me, seems like the workflow was in need of an update. I noticed it's also installing rust via sh.rustup.rs which is interesting considering we typically use another github action for that

@oleonardolima
Copy link
Collaborator Author

This looks good to me, seems like the workflow was in need of an update. I noticed it's also installing rust via sh.rustup.rs which is interesting considering we typically use another github action for that

Oh, you're right. I'll update it to the same one as the other CI jobs.

@oleonardolima oleonardolima force-pushed the fix/ci-code-coverage-upload-artifact-action branch 2 times, most recently from 7b5b4b1 to 8608109 Compare September 4, 2024 16:20
@coveralls
Copy link

coveralls commented Sep 4, 2024

Pull Request Test Coverage Report for Build 10723811847

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 87.438%

Totals Coverage Status
Change from base Build 10690950735: 0.0%
Covered Lines: 1058
Relevant Lines: 1210

💛 - Coveralls

@oleonardolima oleonardolima marked this pull request as ready for review September 4, 2024 17:28
Copy link
Contributor

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

ACK 8608109

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

ACK, mod one comment

- name: Set default toolchain
run: rustup default nightly
- name: Install Rust Toolchain
uses: dtolnay/rust-toolchain@v1
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this change necessary? IIUC, it should do the same thing, but introduces another (unnecessary) dependency to the CI? IMO, the previous way was preferable (and saves the addtional rustup update step later on).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not really necessary. I just used the opportunity to update it to use the same toolchain action used on our other CIs.

I have no big feelings about dropping the e404089 if you folks see fit :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no big feelings about dropping the e404089 if you folks see fit :)

Sounds good, I guess tnull is right

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Awesome, I removed the e404089 commit and rebased it on top of master.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a new commit 7321a5d to fix the tokio-util MSRV problem.

@oleonardolima oleonardolima force-pushed the fix/ci-code-coverage-upload-artifact-action branch from 8608109 to cec57f6 Compare September 5, 2024 15:39
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

ACK 7321a5d

@ValuedMammal
Copy link
Contributor

ACK 7321a5d

@ValuedMammal ValuedMammal merged commit 53db518 into bitcoindevkit:master Sep 7, 2024
25 checks passed
@oleonardolima oleonardolima deleted the fix/ci-code-coverage-upload-artifact-action branch September 9, 2024 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous Integration issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants