feat: overhaul codebase checks #635
Conversation
- feat: add convention plugin for detekt - fix: coverage with sonar - fix: coverage reporting with qodana - chore: establish detekt baselines - chore: refactor ci for efficient check execution - chore: add detekt report upload to ci - chore: update lockfiles Signed-off-by: Sam Gammon <sam@elide.ventures>
sgammon
added
🚧 WIP
WalkthroughWalkthroughThe project underwent comprehensive updates to enhance code quality and maintainability. New workflows for Detekt and Sonar analysis were introduced, alongside adjustments in various Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (12)
config/detekt/baseline.xmlis excluded by:!**/*.xmlconfig/detekt/detekt.ymlis excluded by:!**/*.ymlpackages/cli/detekt-baseline.xmlis excluded by:!**/*.xmlpackages/embedded/detekt-baseline.xmlis excluded by:!**/*.xmlpackages/graalvm-java/detekt-baseline.xmlis excluded by:!**/*.xmlpackages/graalvm-jvm/detekt-baseline.xmlis excluded by:!**/*.xmlpackages/graalvm-kt/detekt-baseline.xmlis excluded by:!**/*.xmlpackages/graalvm-py/detekt-baseline.xmlis excluded by:!**/*.xmlpackages/graalvm-rb/detekt-baseline.xmlis excluded by:!**/*.xmlpackages/graalvm/detekt-baseline.xmlis excluded by:!**/*.xmlpackages/server/detekt-baseline.xmlis excluded by:!**/*.xmltools/processor/detekt-baseline.xmlis excluded by:!**/*.xml
Files selected for processing (31)
- .github/workflows/checks.detekt.yml (1 hunks)
- .github/workflows/checks.sonar.yml (1 hunks)
- .github/workflows/job.test.yml (5 hunks)
- .github/workflows/on.pr.yml (1 hunks)
- build.gradle.kts (8 hunks)
- gradle.properties (1 hunks)
- packages/base/build.gradle.kts (1 hunks)
- packages/base/gradle.lockfile (1 hunks)
- packages/core/gradle.lockfile (1 hunks)
- packages/graalvm-react/build.gradle.kts (1 hunks)
- packages/http/gradle.lockfile (1 hunks)
- packages/model/gradle.lockfile (3 hunks)
- packages/proto/proto-capnp/build.gradle.kts (1 hunks)
- packages/proto/proto-core/gradle.lockfile (1 hunks)
- packages/proto/proto-kotlinx/build.gradle.kts (1 hunks)
- packages/proto/proto-test/gradle.lockfile (5 hunks)
- packages/rpc/gradle.lockfile (1 hunks)
- packages/runtime/gradle.lockfile (1 hunks)
- packages/ssr/gradle.lockfile (1 hunks)
- packages/test/gradle.lockfile (2 hunks)
- tools/elide-build/src/main/kotlin/elide/internal/conventions/Constants.kt (1 hunks)
- tools/elide-build/src/main/kotlin/elide/internal/conventions/ElideBuildExtension.kt (4 hunks)
- tools/elide-build/src/main/kotlin/elide/internal/conventions/ElideConventionPlugin.kt (7 hunks)
- tools/elide-build/src/main/kotlin/elide/internal/conventions/kotlin/KotlinConventions.kt (8 hunks)
- tools/elide-build/src/main/kotlin/elide/internal/conventions/linting/CheckstyleConventionsPlugin.kt (1 hunks)
- tools/elide-build/src/main/kotlin/elide/internal/conventions/linting/DetektConventionsPlugin.kt (1 hunks)
- tools/elide-build/src/main/kotlin/elide/internal/conventions/linting/PmdConventionsPlugin.kt (1 hunks)
- tools/elide-build/src/main/kotlin/elide/internal/conventions/linting/SonarConventionsPlugin.kt (1 hunks)
- tools/elide-build/src/main/kotlin/elide/internal/conventions/linting/SpotlessConventionsPlugin.kt (1 hunks)
- tools/elide-build/src/main/kotlin/elide/internal/conventions/project/ProjectGroups.kt (4 hunks)
- tools/elide-build/src/main/kotlin/elide/internal/conventions/tests/TestConventions.kt (2 hunks)
Files not summarized due to errors (1)
- packages/test/gradle.lockfile: Error: Message exceeds token limit
Files skipped from review due to trivial changes (4)
- packages/proto/proto-capnp/build.gradle.kts
- packages/proto/proto-core/gradle.lockfile
- packages/proto/proto-kotlinx/build.gradle.kts
- packages/rpc/gradle.lockfile
Additional comments: 41
tools/elide-build/src/main/kotlin/elide/internal/conventions/linting/PmdConventionsPlugin.kt (1)
- 19-22: LGTM!
tools/elide-build/src/main/kotlin/elide/internal/conventions/linting/CheckstyleConventionsPlugin.kt (1)
- 19-22: LGTM!
tools/elide-build/src/main/kotlin/elide/internal/conventions/project/ProjectGroups.kt (1)
- 67-67: The removal of various project module lists simplifies project configuration. Ensure that the removal of these lists does not impact any project logic that depended on them.
tools/elide-build/src/main/kotlin/elide/internal/conventions/linting/SpotlessConventionsPlugin.kt (1)
- 1-55: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [22-64]
Ensure that the Spotless configurations align with the project's coding standards and that the specified license header and editorconfig paths exist and are correctly configured.
packages/graalvm-react/build.gradle.kts (1)
- 12-17: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
Ensure the removal of the
elide.internal.conventions.native.NativeTargetimport does not impact the build configuration for native targets.packages/base/build.gradle.kts (1)
- 36-38: The addition of
detektchecks configuration enhances code quality checks. Ensure that the Detekt rules align with the project's coding standards.tools/elide-build/src/main/kotlin/elide/internal/conventions/tests/TestConventions.kt (1)
- 35-52: The addition of logic to copy the Kover test report to a standard path for Gradle improves integration with CI tools. Verify that the specified paths for the Kover report and the destination path are correctly configured and accessible.
tools/elide-build/src/main/kotlin/elide/internal/conventions/linting/DetektConventionsPlugin.kt (1)
- 35-65: Ensure that the Detekt configurations, including parallel execution and report merging, align with the project's requirements and that the specified paths for configuration and reports are correctly set up.
.github/workflows/checks.detekt.yml (1)
- 94-107: Ensure that the Detekt analysis step is correctly configured with the necessary arguments and that the specified paths for caching and reports align with the project's structure.
gradle.properties (1)
- 90-90: The change to
kotlin.stdlib.default.dependencyimpacts the default inclusion of the Kotlin standard library. Verify that this change aligns with the project's dependency management strategy..github/workflows/checks.sonar.yml (1)
- 111-125: The addition of the "Build: Verify Coverage" step before Sonar analysis enhances the workflow by ensuring code coverage verification. Verify that the coverage verification step is correctly configured and that it aligns with the project's quality assurance goals.
packages/runtime/gradle.lockfile (2)
- 39-40: The update to
org.jetbrains.kotlin:kotlin-stdlibversion and its scope adjustment in the lockfile should be verified to ensure compatibility and correct dependency resolution.- 40-40: The adjustment in the scope of
org.jetbrains:annotationsin the lockfile should be verified for correct dependency management and usage within the project.packages/core/gradle.lockfile (1)
- 37-45: The changes from
compileClasspathtoruntimeClasspathfor Kotlin and atomicfu libraries, along with the version update fororg.jetbrains:annotations, are appropriate for ensuring these dependencies are available at runtime without affecting the compile-time classpath. Ensure that this adjustment aligns with the project's dependency management strategy and does not inadvertently affect the build or runtime behavior.tools/elide-build/src/main/kotlin/elide/internal/conventions/linting/SonarConventionsPlugin.kt (1)
- 78-134: The dynamic configuration of SonarQube properties based on project characteristics (multiplatform, JavaScript, JVM) is well-implemented. It ensures that SonarQube analysis is tailored to the specific needs of each project type. Verify that the hardcoded paths and properties used in the configuration align with both SonarQube's expectations and the project's directory layout to avoid potential misconfigurations.
packages/http/gradle.lockfile (1)
- 23-79: The changes from
compileClasspathtoruntimeClasspathfor various libraries, including commons-codec, micronaut, and kotlin libraries, are appropriate for ensuring these dependencies are available at runtime without affecting the compile-time classpath. Ensure that this adjustment aligns with the project's dependency management strategy and does not inadvertently affect the build or runtime behavior.tools/elide-build/src/main/kotlin/elide/internal/conventions/Constants.kt (1)
- 94-95: The addition of a constant for the pinned version of
ktlint(1.1.1) in the Constants object is a good practice for centralizing the management of dependency versions. This approach facilitates easier updates and ensures consistency across the project. Verify that version1.1.1ofktlintmeets the project's current requirements and coding standards.packages/ssr/gradle.lockfile (1)
- 23-84: The shift from
compileClasspathtoruntimeClasspathfor various dependencies is consistent and aligns with best practices for dependency management in Gradle, optimizing the build process and artifact size.packages/base/gradle.lockfile (1)
- 23-75: The shift from
compileClasspathtoruntimeClasspathfor various dependencies is consistent across packages and aligns with best practices for dependency management in Gradle, optimizing the build process and artifact size..github/workflows/on.pr.yml (1)
- 247-259: Renaming the job to "Checks with Detekt" and updating file references and secrets are correctly implemented. The modification of permissions for security events in the "check-detekt" job is appropriate and aligns with the objectives of enhancing code quality checks.
tools/elide-build/src/main/kotlin/elide/internal/conventions/kotlin/KotlinConventions.kt (1)
- 30-68: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [33-201]
Refactoring the
configureKotlinBuildfunction to accept aconventionsparameter of typeElideBuildExtension.Kotlininstead of multiple boolean parameters simplifies the function signature and improves code readability and maintainability. Ensure all configurations previously handled by individual parameters are now correctly handled by theconventionsobject.packages/model/gradle.lockfile (1)
- 63-102: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [6-100]
The shift of dependencies from
compileClasspathtoruntimeClasspathand updates to dependency versions are noted. Ensure that these changes do not introduce any runtime issues or conflicts with other dependencies. Testing the application thoroughly to verify that all dependencies are correctly resolved at runtime is recommended.tools/elide-build/src/main/kotlin/elide/internal/conventions/ElideConventionPlugin.kt (9)
- 19-19: Import of
RedactedGradleSubpluginadded. Verify that the plugin is utilized within the project and that its integration does not introduce any build or runtime issues.- 42-42: Imports for linting conventions added. Ensure that these linting tools are configured correctly and that their rules align with the project's coding standards.
- 50-50: Imports for test conventions added. Confirm that the test configurations and conventions introduced do not negatively impact test execution or coverage reporting.
- 54-54: Import for
configureRedactedPluginadded. Ensure that the Redacted plugin is compatible with the project's Kotlin version and does not interfere with other plugins or build processes.- 83-83: Extension
elideadded to project extensions. Confirm that this extension is used appropriately throughout the project and does not conflict with existing extensions.- 102-108: Configuration for the Redacted plugin within Kotlin conventions. Verify that the plugin's integration is necessary and that it functions as expected without causing build issues.
- 154-157: Kotlin build configuration with specific targets and conventions. Ensure that these configurations are optimal for the project's requirements and do not introduce unnecessary complexity.
- 162-165: Integration of Kotlin linting tools. Confirm that these tools are configured with appropriate rules and do not produce false positives that could disrupt development workflows.
- 226-227: Configuration for Kover within testing conventions. Verify that Kover is correctly configured for accurate coverage reporting and does not conflict with other testing tools.
tools/elide-build/src/main/kotlin/elide/internal/conventions/ElideBuildExtension.kt (2)
- 84-95: New configuration options for Redacted, experimental JS objects, SAM-with-receiver, and power-assert plugins added to the Kotlin convention. Ensure that these options are necessary for the project and that their inclusion does not introduce build or runtime issues.
- 137-155: Introduction of a
Checksclass with options for Detekt, Sonar, Checkstyle, PMD, linting failures, and baselines. Confirm that these checks are configured correctly and align with the project's quality standards..github/workflows/job.test.yml (1)
- 240-240: Ensure that the
SONAR_TOKENis securely handled and not exposed in logs or error messages.Verification successful
Given the script's execution and its lack of output, there's no direct evidence of
SONAR_TOKENbeing exposed throughechocommands in the workflow logs. This suggests that, at least in the context of usingecho, the token is not being accidentally exposed. However, it's important to note that this does not fully guarantee the secure handling ofSONAR_TOKENacross all possible logging mechanisms or error messages. The verification focused on a specific potential exposure method and found no issues within that scope.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for accidental exposure of SONAR_TOKEN in workflow logs rg 'SONAR_TOKEN' --files-with-matches | xargs grep -n 'echo'Length of output: 58
build.gradle.kts (6)
- 20-21: Imports for
DetektandReportMergeTaskare added. Verify that these imports are used effectively within the file and that the Detekt plugin is applied correctly.Verification successful
Based on the provided output, the Detekt plugin and the imported classes (
DetektandReportMergeTask) are referenced in thebuild.gradle.ktsfile and across multiple other files within the project. This indicates that the Detekt plugin is integrated into the project's build process, and the imports are likely used effectively within thebuild.gradle.ktsfile and potentially in related configurations. To confirm the specific usage and effectiveness of these imports, direct inspection of thebuild.gradle.ktsfile and related configurations would be necessary. However, the evidence suggests that the review comment is addressed by the project's current state.* 282-290: The Detekt configuration uses a baseline file and builds upon the default config. Ensure that the baseline file (`config/detekt/baseline.xml`) exists and is up-to-date with the current codebase to avoid ignoring new issues.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify Detekt plugin application and usage of imported classes rg 'apply.*detekt' --files-with-matches rg 'Detekt|ReportMergeTask' --files-with-matchesLength of output: 842
Verification successful
The verification process confirms that the Detekt baseline file (
config/detekt/baseline.xml) exists and has been recently updated on Feb 15, which aligns with the current date. This ensures the file is up-to-date with the current codebase, addressing the concern raised in the review comment.* 298-321: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [292-310]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existence and recent update of Detekt baseline file fd 'baseline.xml' --exec ls -lLength of output: 882
Sonar properties are updated to include paths for various reports. Ensure that the specified paths for reports like
sonar.kotlin.detekt.reportPathsactually exist and are generated by the build process.
- 318-318: Dependencies for Kover are updated. Confirm that all listed projects have Kover configured and that coverage reporting is correctly integrated into the CI process.
- 362-373: The
detekttasks are finalized bydetektMergeSarifanddetektMergeXml. Ensure that these finalization tasks are defined and correctly merge Detekt reports into the specified formats.Verification successful
The verification process confirms that the
detektMergeSarifanddetektMergeXmltasks are defined and correctly configured in bothbuild.gradle.ktsandDetektConventionsPlugin.kt. These tasks are registered with their output paths set appropriately, and they are finalized by the Detekt tasks, ensuring that the Detekt reports are merged into the specified SARIF and XML formats after analysis. The required reports are enabled, and the merge tasks are configured to use the Detekt tasks' output files as input, aligning with the intended functionality. Therefore, the review comment is verified as accurate.* 575-577: The `koverVerify` task is finalized by `copyCoverageReports`. Ensure that the `copyCoverageReports` task correctly copies coverage reports to the specified location for Qodana analysis.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify definition and correct configuration of detektMergeSarif and detektMergeXml tasks rg 'detektMergeSarif|detektMergeXml' --files-with-matchesLength of output: 172
Script:
#!/bin/bash # Extract and review the sections related to detektMergeSarif and detektMergeXml in build.gradle.kts echo "Contents from build.gradle.kts:" rg 'detektMergeSarif|detektMergeXml' build.gradle.kts --context 5 echo -e "\nContents from DetektConventionsPlugin.kt:" rg 'detektMergeSarif|detektMergeXml' tools/elide-build/src/main/kotlin/elide/internal/conventions/linting/DetektConventionsPlugin.kt --context 5Length of output: 2244
Verification successful
The
copyCoverageReportstask is configured to copy coverage reports to the root project for Qodana analysis, as described. It depends onkoverBinaryReportandkoverXmlReport, and copiesreport.bin,report.xml, andverify.errfrom thereports/koverdirectory. This setup aligns with the intent to ensure that thecopyCoverageReportstask correctly supports Qodana analysis by providing necessary coverage reports.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check configuration of copyCoverageReports task rg 'copyCoverageReports' --files-with-matchesLength of output: 62
Script:
#!/bin/bash # Extract and display the configuration of the 'copyCoverageReports' task from 'build.gradle.kts' rg 'copyCoverageReports' build.gradle.kts -A 10Length of output: 606
packages/proto/proto-test/gradle.lockfile (1)
- 274-324: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [4-330]
The changes in dependency classifications from
compileClasspathtoruntimeClasspathneed justification. Ensure these changes do not affect the compile-time requirements and that the runtime dependencies are indeed necessary for execution. This adjustment can impact the build process and application performance.
- feat: add convention plugin for detekt - fix: coverage with sonar - fix: coverage reporting with qodana - chore: establish detekt baselines - chore: refactor ci for efficient check execution - chore: add detekt report upload to ci - chore: update lockfiles Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring: Next stepsTake a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with |
|
@SocketSecurity ignore npm/v8-profiler-next@1.10.0 |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
kotlin-js-store/package-lock.jsonis excluded by:!**/*.json
Files selected for processing (5)
- .github/workflows/job.test.yml (5 hunks)
- build.gradle.kts (9 hunks)
- gradle.properties (1 hunks)
- npm (1 hunks)
- packages/embedded/build.gradle.kts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- .github/workflows/job.test.yml
- gradle.properties
Additional comments: 11
npm (1)
- 1-3: The changes to the script are correct and follow best practices.
build.gradle.kts (9)
- 20-21: Ensure the newly added imports are used within the file to avoid unnecessary imports that can clutter the namespace.
- 171-177: The configuration for Idea project settings is correctly updated to reflect the Java language version specified in project properties.
- 179-186: The Nexus publishing configuration is correctly set up with the new Sonatype URLs. Ensure the credentials for Sonatype are securely managed and not hardcoded in the build scripts.
- 264-272: The Detekt configuration is comprehensive, including parallel execution and compiler plugin enabling. Verify that the
detekt.ymlandbaseline.xmlfiles exist at the specified paths and are correctly configured.Verification successful
The verification process confirmed that both
detekt.ymlandbaseline.xmlfiles exist at the specified paths. This aligns with the review comment's request to verify their existence. Without specific criteria for what constitutes correct configuration, the verification of their existence completes the task at hand.* 280-303: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [274-292]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if Detekt configuration files exist test -f config/detekt/detekt.yml && echo "Detekt config exists" || echo "Detekt config missing" test -f config/detekt/baseline.xml && echo "Detekt baseline exists" || echo "Detekt baseline missing"Length of output: 177
The Sonar configuration correctly sets project key, organization, and coverage report paths. Ensure that the empty report paths for Checkstyle and ktlint are intentional and that these tools are either not used or their reports are generated elsewhere.
- 375-381: The tasks for merging Detekt reports (
detektMergeSarifanddetektMergeXml) are correctly configured with their output paths. Ensure that these tasks are called as part of the CI pipeline or build process to aggregate Detekt findings.- 383-387: The configuration for Detekt tasks includes finalization by the merge tasks and requires both SARIF and XML reports. This setup ensures that Detekt findings are properly aggregated and available in both formats for further analysis.
- 504-519: The
copyCoverageReportstask is correctly set up to copy Kover reports to a directory for Qodana analysis. Ensure that this task is integrated into the CI pipeline to facilitate code quality checks with Qodana.- 580-587: The Sonar task dependencies are correctly configured to ensure that all necessary reports and verifications are completed before Sonar analysis. This setup helps maintain the integrity and relevance of the Sonar analysis results.
packages/embedded/build.gradle.kts (1)
- 1103-1108: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The removal of
outputs.upToDateWhen { false }from thetesttask configuration is not directly visible in the provided code. Ensure this change aligns with the project's goals for build performance and correctness.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #635 +/- ##
=======================================
Coverage 36.96% 36.96%
=======================================
Files 469 469
Lines 13885 13885
Branches 1954 1954
=======================================
Hits 5133 5133
Misses 8403 8403
Partials 349 349
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
Signed-off-by: Sam Gammon <sam@elide.ventures>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Summary
Continued tooling cleanup and updates, this time focused on checks (locally and in CI).
Still pending
Changelog
Summary by CodeRabbit
New Features
build.gradle.ktsfiles.Improvements
SONAR_TOKENas a required parameter and updated environment variables.Bug Fixes
Documentation
Refactor
build.gradle.ktsfiles.Style
Tests
Chores
gradle.lockfilefor several packages.Revert