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 cargo check for all features #121

Merged

Conversation

AkhilTThomas
Copy link
Contributor

@AkhilTThomas AkhilTThomas commented Dec 13, 2024

This PR adds cargo-hack to build each target with all the possible feature flags.
Addresses #120

Copy link

codecov bot commented Dec 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.58%. Comparing base (67fc220) to head (c344974).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #121   +/-   ##
=======================================
  Coverage   59.58%   59.58%           
=======================================
  Files          33       33           
  Lines       16071    16071           
=======================================
  Hits         9576     9576           
  Misses       6495     6495           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AkhilTThomas AkhilTThomas force-pushed the fix-broke-no-default-feat branch 2 times, most recently from 9808b36 to 71e1b67 Compare December 13, 2024 18:23
@AkhilTThomas
Copy link
Contributor Author

Caught an issue in databroker-cli for --no-default-features
So it should address #120

@AkhilTThomas AkhilTThomas marked this pull request as ready for review December 16, 2024 07:29
* Run cargo check on each feature for all bins
@AkhilTThomas AkhilTThomas force-pushed the fix-broke-no-default-feat branch from 107d1b5 to 6c1eab6 Compare December 22, 2024 05:49
@SebastianSchildt
Copy link
Contributor

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅

120 - Fully compliant

Fully compliant requirements:

  • Ensure that the no-default feature (disabling TLS support) is built in CI.
  • Detect breakage in the no-default feature build.

Not compliant requirements:
[]

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Conditional Compilation

The addition of the #[cfg(feature = "tls")] attribute to the get_ca_cert method may cause unexpected behavior if the feature flag is not properly managed or tested. Ensure this conditional compilation is correctly handled.

#[cfg(feature = "tls")]
pub fn get_ca_cert(&mut self) -> Option<String> {
    self.ca_cert.clone()
}
CI Feature Check

The addition of cargo hack check --each-feature ensures feature flag testing, but it should be validated that all relevant features are being tested and no critical features are missed.

- name: "Check each feature"
  working-directory: ${{github.workspace}}
  run: cargo hack check --each-feature

@@ -173,6 +176,10 @@ jobs:
run: |
./scripts/build-databroker.sh ${{ matrix.platform.name }}

- name: "Check each feature"
working-directory: ${{github.workspace}}
run: cargo hack check --each-feature
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems we run this for each platform in the build matrix, but I think we do nothing platform specific here, so maybe moving it out can save some time?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe in the (unit) test job?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SebastianSchildt This adds ~5m 12s runtime for the unit test action.

Copy link
Contributor

@SebastianSchildt SebastianSchildt left a comment

Choose a reason for hiding this comment

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

Nice. Looking good. As for the runtime, this should also profit from the caching. We can come back if it gets too long/annoying

🏖️

@SebastianSchildt SebastianSchildt merged commit 7911268 into eclipse-kuksa:main Jan 2, 2025
23 checks passed
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