-
Notifications
You must be signed in to change notification settings - Fork 0
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
extend ci #10
extend ci #10
Conversation
kod-kristoff
commented
Mar 20, 2024
- ci: copy from jonhoo/rust-ci-conf
- ci: remove nostd safety
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @kod-kristoff - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Hard-coded secret detected. (link)
General suggestions:
- Review the pinning of GitHub Actions to specific commits or tags to ensure stability and reproducibility in CI workflows.
- Evaluate the security implications of introducing third-party GitHub Actions and ensure they comply with the project's security policies.
- Consider the impact of treating deprecation warnings as errors in CI on the development workflow and dependency update strategy.
- Ensure sensitive information, such as tokens, is securely managed using GitHub Secrets rather than being hard-coded in workflow files.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🔴 Security: 1 blocking issue, 2 other issues
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
- name: Install ${{ matrix.toolchain }} | ||
uses: dtolnay/rust-toolchain@master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code_refinement): Consider pinning the rust-toolchain action to a specific commit or tag.
Using a branch reference like 'master' can lead to unexpected changes in your CI pipeline when the action is updated. Pinning to a specific commit or tag ensures reproducibility and stability.
toolchain: [stable, beta] | ||
steps: | ||
- uses: actions/checkout@v4 | ||
with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 suggestion (security): Ensure that the usage of third-party actions aligns with security policies.
Using third-party GitHub Actions like 'codecov/codecov-action@v4' introduces external dependencies into your CI/CD pipeline. It's important to review and ensure these actions comply with your project's security policies and practices.
env: | ||
RUSTFLAGS: -D deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code_refinement): Consider the implications of treating deprecation warnings as errors in CI.
Using -D deprecated
treats all deprecation warnings as errors, which can be beneficial for keeping the codebase modern and clean. However, it might also introduce friction when dependencies deprecate features. Ensure this aligns with your development workflow and dependency update strategy.
.github/workflows/test.yml
Outdated
# direct dependencies of this crate, while selecting the maximal versions for the transitive | ||
# dependencies. Alternatively, you can add a line in your Cargo.toml to artificially increase | ||
# the minimal dependency, which you do with e.g.: | ||
# ```toml | ||
# # for minimal-versions | ||
# [target.'cfg(any())'.dependencies] | ||
# openssl = { version = "0.10.55", optional = true } # needed to allow foo to build with -Zminimal-versions | ||
# ``` | ||
# The optional = true is necessary in case that dependency isn't otherwise transitively required | ||
# by your library, and the target bit is so that this dependency edge never actually affects | ||
# Cargo build order. See also | ||
# https://github.com/jonhoo/fantoccini/blob/fde336472b712bc7ebf5b4e772023a7ba71b2262/Cargo.toml#L47-L49. | ||
# This action is run on ubuntu with the stable toolchain, as it is not expected to fail | ||
runs-on: ubuntu-latest | ||
name: ubuntu / stable / minimal-versions | ||
steps: | ||
- uses: actions/checkout@v4 | ||
with: | ||
submodules: true | ||
- name: Install stable | ||
uses: dtolnay/rust-toolchain@stable | ||
- name: Install nightly for -Zminimal-versions | ||
uses: dtolnay/rust-toolchain@nightly | ||
- name: rustup default stable | ||
run: rustup default stable | ||
- name: cargo update -Zminimal-versions | ||
run: cargo +nightly update -Zdirect-minimal-versions | ||
- name: cargo test | ||
run: cargo test --locked --all-features --all-targets | ||
os-check: | ||
# run cargo test on mac and windows | ||
runs-on: ${{ matrix.os }} | ||
name: ${{ matrix.os }} / stable | ||
strategy: | ||
fail-fast: false | ||
matrix: | ||
os: [macos-latest, windows-latest] | ||
steps: | ||
# if your project needs OpenSSL, uncomment this to fix Windows builds. | ||
# it's commented out by default as the install command takes 5-10m. | ||
# - run: echo "VCPKG_ROOT=$env:VCPKG_INSTALLATION_ROOT" | Out-File -FilePath $env:GITHUB_ENV -Append | ||
# if: runner.os == 'Windows' | ||
# - run: vcpkg install openssl:x64-windows-static-md | ||
# if: runner.os == 'Windows' | ||
- uses: actions/checkout@v4 | ||
with: | ||
submodules: true | ||
- name: Install stable | ||
uses: dtolnay/rust-toolchain@stable | ||
- name: cargo generate-lockfile | ||
if: hashFiles('Cargo.lock') == '' | ||
run: cargo generate-lockfile | ||
- name: cargo test | ||
run: cargo test --locked --all-features --all-targets | ||
coverage: | ||
# use llvm-cov to build and collect coverage and outputs in a format that | ||
# is compatible with codecov.io | ||
# | ||
# note that codecov as of v4 requires that CODECOV_TOKEN from | ||
# | ||
# https://app.codecov.io/gh/<user or org>/<project>/settings | ||
# | ||
# is set in two places on your repo: | ||
# | ||
# - https://github.com/jonhoo/guardian/settings/secrets/actions | ||
# - https://github.com/jonhoo/guardian/settings/secrets/dependabot | ||
# | ||
# (the former is needed for codecov uploads to work with Dependabot PRs) | ||
# | ||
# PRs coming from forks of your repo will not have access to the token, but | ||
# for those, codecov allows uploading coverage reports without a token. | ||
# it's all a little weird and inconvenient. see | ||
# | ||
# https://github.com/codecov/feedback/issues/112 | ||
# | ||
# for lots of more discussion | ||
runs-on: ubuntu-latest | ||
name: ubuntu / stable / coverage | ||
steps: | ||
- uses: actions/checkout@v4 | ||
with: | ||
submodules: true | ||
- name: Install stable | ||
uses: dtolnay/rust-toolchain@stable | ||
with: | ||
components: llvm-tools-preview | ||
- name: cargo install cargo-llvm-cov | ||
uses: taiki-e/install-action@cargo-llvm-cov | ||
- name: cargo generate-lockfile | ||
if: hashFiles('Cargo.lock') == '' | ||
run: cargo generate-lockfile | ||
- name: cargo llvm-cov | ||
run: cargo llvm-cov --locked --all-features --lcov --output-path lcov.info | ||
- name: Record Rust version | ||
run: echo "RUST=$(rustc --version)" >> "$GITHUB_ENV" | ||
- name: Upload to codecov.io | ||
uses: codecov/codecov-action@v4 | ||
with: | ||
fail_ci_if_error: true | ||
token: ${{ secrets.CODECOV_TOKEN }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 issue (security): Hard-coded secret detected.
It's recommended to avoid hard-coding secrets and tokens in workflow files. Consider using GitHub Secrets to store sensitive information securely.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
|