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

ci: add check for feature flags #918

Merged
merged 16 commits into from
May 29, 2024
Merged

ci: add check for feature flags #918

merged 16 commits into from
May 29, 2024

Conversation

mayconamaroCW
Copy link
Contributor

@mayconamaroCW mayconamaroCW commented May 24, 2024

Creates just check-features and run it on CI

@mayconamaroCW mayconamaroCW requested a review from a team as a code owner May 24, 2024 14:42
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

2, because the PR involves adding a new CI workflow and a new command in the justfile. The changes are straightforward and mostly involve configuration, which is generally easier to review than complex logic changes.

🧪 Relevant tests

No

⚡ Possible issues

Possible Redundancy: The installation of cargo-hack is done both in the GitHub Actions workflow and in the justfile. This could lead to unnecessary redundancy and increased build times.

🔒 Security concerns

No

Code feedback:
relevant file.github/workflows/check-features.yml
suggestion      

Consider using a matrix strategy for different operating systems or Rust versions to ensure broader compatibility. This can be particularly useful if you plan to support multiple environments or anticipate future expansions. [medium]

relevant lineruns-on: ubuntu-latest

relevant file.github/workflows/check-features.yml
suggestion      

It's recommended to update the actions/checkout and actions/cache to their latest versions to take advantage of any fixes and new features. This can help avoid potential issues with older versions. [important]

relevant lineuses: actions/checkout@v2

relevant filejustfile
suggestion      

To avoid potential failures during parallel executions, consider adding a lock or check to ensure that cargo-hack is not installed multiple times if it's already being used or installed elsewhere in your scripts. [medium]

relevant linecommand -v cargo-hack >/dev/null 2>&1 || { cargo install cargo-hack; }

relevant file.github/workflows/check-features.yml
suggestion      

Introduce caching for the installation of cargo-hack and other tools to speed up the workflow execution. This can be done by modifying the cache key to include hashes of relevant configuration files or tool versions. [important]

relevant linekey: ${{ runner.os }}-v2-cargo-stable-release-${{ hashFiles('Cargo.lock', 'Cargo.toml') }}

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Performance
Improve cache key specificity to avoid collisions and ensure cache relevance

Use a more specific cache key to avoid cache collisions and ensure that the cache is
relevant to the current state of dependencies. This can be achieved by including more
specific elements in the cache key.

.github/workflows/check-features.yml [33]

-key: ${{ runner.os }}-v2-cargo-stable-release-${{ hashFiles('Cargo.lock', 'Cargo.toml') }}
+key: ${{ runner.os }}-v2-cargo-${{ github.sha }}-${{ hashFiles('Cargo.lock', 'Cargo.toml') }}
 
Suggestion importance[1-10]: 9

Why: Improving cache key specificity can significantly reduce cache collisions and ensure that the cache is always relevant, enhancing performance and reliability.

9
Use a Docker container to reduce setup time and ensure consistent environments

Instead of installing dependencies conditionally based on the cache hit status, consider
using a Docker container with these dependencies pre-installed. This approach can reduce
the setup time for each job run.

.github/workflows/check-features.yml [37-39]

-- name: Set up Rust
-  if: ${{ steps.cache-cargo.outputs.cache-hit != 'true' }}
-  run: curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y
+- name: Use Docker Container
+  uses: docker://rust:latest
 
Suggestion importance[1-10]: 7

Why: Using a Docker container can reduce setup time and ensure consistent environments, but it may introduce additional complexity in managing Docker images.

7
Enhancement
Limit CI runs to specific branches to save resources

Consider specifying the exact branches for the workflow trigger to avoid unnecessary runs
on all branches. This can help in reducing CI minutes and focusing on the branches that
are most relevant for feature checks.

.github/workflows/check-features.yml [6]

 branches:
-  - '*'
+  - 'main'
+  - 'develop'
 
Suggestion importance[1-10]: 8

Why: Limiting CI runs to specific branches can save resources and reduce unnecessary checks, making the CI process more efficient.

8
Maintainability
Ensure necessary tools like cargo-hack are pre-installed in the CI setup to streamline scripts

Instead of checking for the presence of cargo-hack every time before running the command,
consider adding a setup step in your CI pipeline to ensure that cargo-hack is installed.
This will make the script cleaner and more efficient.

justfile [55]

-command -v cargo-hack >/dev/null 2>&1 || { cargo install cargo-hack; }
+# Assuming cargo-hack is installed in the CI setup
 
Suggestion importance[1-10]: 6

Why: Pre-installing necessary tools in the CI setup can streamline scripts and improve maintainability, though it requires ensuring the CI environment is correctly configured.

6

@mayconamaroCW mayconamaroCW enabled auto-merge (squash) May 29, 2024 14:55
@mayconamaroCW mayconamaroCW disabled auto-merge May 29, 2024 16:21
@mayconamaroCW mayconamaroCW enabled auto-merge (squash) May 29, 2024 17:22
@mayconamaroCW mayconamaroCW merged commit f5be427 into main May 29, 2024
32 checks passed
@mayconamaroCW mayconamaroCW deleted the ci/check-features branch May 29, 2024 17:29
@mayconamaroCW mayconamaroCW linked an issue Jun 4, 2024 that may be closed by this pull request
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.

run cargo check with all features
2 participants