Enable more modules in our CI + fix missing triggers #308
Conversation
WalkthroughThe pull request modifies the GitHub Actions workflows for Scala and Python testing by removing specific path filters from the Changes
Possibly related PRs
Suggested Reviewers
Poem
Warning Review ran into problems🔥 ProblemsGitHub Actions: Resource not accessible by integration - https://docs.github.com/rest/actions/workflow-runs#list-workflow-runs-for-a-repository. Please grant the required permissions to the CodeRabbit GitHub App under the organization or repository settings. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (10)
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
piyush-zlai
force-pushed
the
piyush/enable_gcp_ci
branch
from
13e4800 to
f538e59
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/test_scala_no_spark.yaml (1)
96-114: Fix trailing whitespace in test commands.Remove trailing spaces in the following lines:
- Line 109:
sbt "++ 2.12.18 cloud_gcp/test"- Line 114:
sbt "++ 2.12.18 cloud_aws/test"- sbt "++ 2.12.18 cloud_gcp/test" + sbt "++ 2.12.18 cloud_gcp/test" - sbt "++ 2.12.18 cloud_aws/test" + sbt "++ 2.12.18 cloud_aws/test"🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 109-109: trailing spaces
(trailing-spaces)
[error] 114-114: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
.github/workflows/test_scala_no_spark.yaml(3 hunks)cloud_gcp/src/test/scala/ai/chronon/integrations/cloud_gcp/BigTableKVStoreTest.scala(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/test_scala_no_spark.yaml
[error] 109-109: trailing spaces
(trailing-spaces)
[error] 114-114: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: no_spark_scala_tests
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (4)
cloud_gcp/src/test/scala/ai/chronon/integrations/cloud_gcp/BigTableKVStoreTest.scala (3)
33-46: LGTM! Clean migration to ScalaTest.The shift from JUnit4 to ScalaTest's BeforeAndAfter trait with bundled emulator setup improves test isolation.
Line range hint
48-61: LGTM! Consistent emulator configuration.The emulator port configuration is correctly applied to both data and admin settings.
Line range hint
67-350: LGTM! Comprehensive test coverage.Test suite covers CRUD operations, error handling, and time series scenarios thoroughly.
.github/workflows/test_scala_no_spark.yaml (1)
12-17: LGTM! Path triggers correctly configured.New paths for cloud modules are properly added to both push and PR triggers.
Also applies to: 28-33
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/test_scala_no_spark.yaml (1)
79-87: Fix trailing spaces in sbt commands.- sbt "++ 2.12.18 cloud_gcp/test" + sbt "++ 2.12.18 cloud_gcp/test" - sbt "++ 2.12.18 cloud_aws/test" + sbt "++ 2.12.18 cloud_aws/test"🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 82-82: trailing spaces
(trailing-spaces)
[error] 87-87: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
.github/workflows/test_python.yaml(0 hunks).github/workflows/test_scala_no_spark.yaml(1 hunks).github/workflows/test_scala_spark.yaml(0 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/test_python.yaml
- .github/workflows/test_scala_spark.yaml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/test_scala_no_spark.yaml
[error] 82-82: trailing spaces
(trailing-spaces)
[error] 87-87: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: no_spark_scala_tests
- GitHub Check: table_utils_delta_format_spark_tests
- GitHub Check: mutation_spark_tests
- GitHub Check: join_spark_tests
- GitHub Check: fetcher_spark_tests
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: other_spark_tests
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (2)
.github/workflows/test_scala_no_spark.yaml (2)
Line range hint
3-11: LGTM! Removing path filters ensures comprehensive CI coverage.Removing specific path filters ensures PRs can't bypass checks.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 82-82: trailing spaces
(trailing-spaces)
[error] 87-87: trailing spaces
(trailing-spaces)
69-72: LGTM! Good addition of service_commons tests.Memory settings match existing steps.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/test_scala_no_spark_2.yaml (3)
30-33: Consider using environment file for SBT_OPTS.The SBT_OPTS setting is duplicated across test steps.
- name: Set SBT resources run: | echo "SBT_OPTS=-Xmx8G -Xms2G" >> $GITHUB_ENV + echo "SCALA_VERSION=2.12.18" >> $GITHUB_ENVThen use
${{ env.SBT_OPTS }}and${{ env.SCALA_VERSION }}in test steps.
57-57: Fix trailing whitespace.Remove trailing spaces from lines 57 and 62.
- sbt "++ 2.12.18 cloud_gcp/test" + sbt "++ 2.12.18 cloud_gcp/test" - sbt "++ 2.12.18 cloud_aws/test" + sbt "++ 2.12.18 cloud_aws/test"Also applies to: 62-62
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 57-57: trailing spaces
(trailing-spaces)
34-62: Consider parallelizing test execution.Running tests sequentially might increase workflow duration.
Consider using matrix strategy to run tests in parallel:
strategy: matrix: module: [hub, service, service_commons, orchestration, cloud_gcp, cloud_aws] steps: - name: Run ${{ matrix.module }} tests run: | sbt "++ 2.12.18 ${{ matrix.module }}/test"🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 57-57: trailing spaces
(trailing-spaces)
[error] 62-62: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
.github/workflows/test_scala_no_spark.yaml(1 hunks).github/workflows/test_scala_no_spark_2.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test_scala_no_spark.yaml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/test_scala_no_spark_2.yaml
[error] 57-57: trailing spaces
(trailing-spaces)
[error] 62-62: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: table_utils_delta_format_spark_tests
- GitHub Check: join_spark_tests
- GitHub Check: fetcher_spark_tests
- GitHub Check: other_spark_tests
- GitHub Check: mutation_spark_tests
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: no_spark_scala_tests
- GitHub Check: no_spark_scala_tests
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (2)
.github/workflows/test_scala_no_spark_2.yaml (2)
1-13: LGTM: Workflow configuration is well-structured.Concurrency settings will help manage resources efficiently.
15-26: LGTM: Job configuration uses appropriate container setup.Container credentials and workspace settings are properly configured.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
.github/image/Dockerfile(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: table_utils_delta_format_spark_tests
- GitHub Check: other_spark_tests
- GitHub Check: mutation_spark_tests
- GitHub Check: join_spark_tests
- GitHub Check: no_spark_scala_tests
- GitHub Check: fetcher_spark_tests
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: no_spark_scala_tests
🔇 Additional comments (1)
.github/image/Dockerfile (1)
8-8: LGTM: OpenJDK installation looks good.
.github/image/Dockerfile
Outdated
|
|
||
| # java | ||
| ENV JAVA_HOME=/usr/lib/jvm/default-jvm | ||
| ENV JAVA_HOME=/usr/lib/jvm/java-17-openjdk-amd64 |
There was a problem hiding this comment.
Fix JAVA_HOME path for Alpine.
The path is incorrect for Alpine Linux.
-ENV JAVA_HOME=/usr/lib/jvm/java-17-openjdk-amd64
+ENV JAVA_HOME=/usr/lib/jvm/java-17-openjdk📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ENV JAVA_HOME=/usr/lib/jvm/java-17-openjdk-amd64 | |
| ENV JAVA_HOME=/usr/lib/jvm/java-17-openjdk |
|
Killing this as we're cutting over to bazel.. |
Summary
Noticed that some of our modules are missing in CI (e.g. gcp and aws). Also some of our path triggers were missing some locations so we might have prs slipping through with bugs.
Checklist
Summary by CodeRabbit