-
Notifications
You must be signed in to change notification settings - Fork 9
Updated dev notes with Bazel installation instructions and java error fix #322
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
Conversation
WalkthroughThe changes update the Changes
Possibly related PRs
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
Warning Review ran into problems🔥 ProblemsGitHub Actions and Pipeline Checks: 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. 🪧 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 (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tools/build_rules/thrift/thrift.bzl (1)
103-114: Consider adding language support configuration.The TODO indicates plans for multi-language support. Consider adding a
languagesattribute to configure which languages to generate.def thrift_gen_library(name, srcs, **kwargs): + languages = kwargs.pop("languages", ["java"]) _thrift_gen_library( name = name, srcs = srcs, + languages = languages, thrift_binary = select({devnotes.md (2)
68-77: IntelliJ Configuration Instructions.
Use asterisks for list items to meet markdown style guidelines.🧰 Tools
🪛 LanguageTool
[uncategorized] ~74-~74: Consider adding a hyphen.
Context: ...ile - We should see a bazel icon in the top right corner to the left of search bar - U...(TOP_LEFT_CORNER)
🪛 markdownlint-cli2 (0.17.2)
70-70: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
71-71: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
72-72: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
72-72: Unordered list indentation
Expected: 2; Actual: 3(MD007, ul-indent)
73-73: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
73-73: Unordered list indentation
Expected: 2; Actual: 3(MD007, ul-indent)
74-74: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
75-75: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
75-75: Unordered list indentation
Expected: 2; Actual: 3(MD007, ul-indent)
76-76: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
123-169: Legacy SBT Setup Section Present.
Retaining old SBT instructions. Confirm this is intentional for legacy support.🧰 Tools
🪛 LanguageTool
[grammar] ~135-~135: Did you mean “copying and pasting”?
Context: ...17-fails-with-illegalaccesserror-cl) by copy pasting into the run configuration arguments li...(COPY_PASTE)
🪛 markdownlint-cli2 (0.17.2)
127-127: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
128-128: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
129-129: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
130-130: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
133-133: Images should have alternate text (alt text)
null(MD045, no-alt-text)
151-151: Images should have alternate text (alt text)
null(MD045, no-alt-text)
154-154: Images should have alternate text (alt text)
null(MD045, no-alt-text)
155-155: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
156-156: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
161-161: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
164-164: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (14)
.github/workflows/test_scala_no_spark.yaml(1 hunks).github/workflows/test_scala_spark.yaml(1 hunks)aggregator/BUILD.bazel(3 hunks)api/BUILD.bazel(2 hunks)cloud_gcp/BUILD.bazel(3 hunks)devnotes.md(1 hunks)flink/BUILD.bazel(4 hunks)hub/BUILD.bazel(3 hunks)online/BUILD.bazel(3 hunks)orchestration/BUILD.bazel(3 hunks)service/BUILD.bazel(1 hunks)service_commons/BUILD.bazel(1 hunks)spark/BUILD.bazel(3 hunks)tools/build_rules/thrift/thrift.bzl(3 hunks)
🧰 Additional context used
🪛 LanguageTool
devnotes.md
[uncategorized] ~74-~74: Consider adding a hyphen.
Context: ...ile - We should see a bazel icon in the top right corner to the left of search bar - U...
(TOP_LEFT_CORNER)
🪛 markdownlint-cli2 (0.17.2)
devnotes.md
70-70: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
71-71: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
72-72: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
72-72: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
73-73: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
73-73: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
74-74: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
75-75: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
75-75: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
76-76: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
🔇 Additional comments (30)
cloud_gcp/BUILD.bazel (4)
7-7: LGTM: Dependency update aligns with project-wide standardization.The change from
//api:thriftto//api:thrift_javamatches the standardization effort.
49-49: LGTM: Consistent dependency update in test_deps.Matches the same dependency update as in cloud_gcp_lib.
87-87: LGTM: Library name follows new convention.Renaming from cloud_gcp_test_lib to test_lib aligns with project standards.
95-95: LGTM: Test suite updates are consistent.Both the target name change to "tests" and the dependency update to ":test_lib" follow the standardization.
Also applies to: 100-100
orchestration/BUILD.bazel (1)
7-7: LGTM! Consistent dependency and naming updates.Also applies to: 17-17, 28-28, 35-35, 38-38
service_commons/BUILD.bazel (1)
21-21: LGTM! Consistent with project-wide updates.Also applies to: 29-29, 36-36, 40-40
service/BUILD.bazel (1)
35-35: LGTM! Naming standardization applied correctly.Also applies to: 42-42, 46-46
aggregator/BUILD.bazel (1)
7-7: LGTM! All changes align with project standards.Also applies to: 23-23, 50-50, 57-57, 60-60
api/BUILD.bazel (3)
8-19: LGTM: Explicit java_library naming.Clear dependency separation between generated code and Java sources.
21-60: LGTM: Consistent dependency and naming updates.
1-6: LGTM: New thrift_gen_library rule looks good.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Verify thrift files exist fd -e thrift . thrift/Length of output: 119
I'll re-run the verification by checking for thrift files relative to the BUILD file’s directory. Let's execute a script that looks into the
api/thriftfolder:
🏁 Script executed:
#!/bin/bash # Check for thrift files under api/thrift (BUILD file location) if [ -d "api/thrift" ]; then find api/thrift -type f -name "*.thrift" else echo "Directory api/thrift does not exist" fiLength of output: 225
Thrift Gen Rule Verified
api/BUILD.bazelcorrectly usesglob(["thrift/*.thrift"])relative to the BUILD file.- The
api/thriftdirectory exists and contains the expected thrift files.hub/BUILD.bazel (1)
1-71: LGTM: Consistent dependency and naming updates.online/BUILD.bazel (1)
1-78: LGTM: Consistent dependency and naming updates.flink/BUILD.bazel (1)
1-92: LGTM: Improved naming clarity with module prefixes.Binary names now clearly indicate their module context.
tools/build_rules/thrift/thrift.bzl (2)
50-55: LGTM! Function renamed for better extensibility.The implementation function is now named more generically.
87-101: LGTM! Rule renamed consistently.The rule name matches the implementation function.
spark/BUILD.bazel (3)
8-8: LGTM! Dependency updated.Updated to use the new Java-specific Thrift library.
89-93: LGTM! Test naming standardized.Test library and suite names follow the new convention.
Also applies to: 95-103
105-110: LGTM! Binary name clarified.More descriptive name for the Spark assembly.
.github/workflows/test_scala_spark.yaml (1)
47-47: LGTM! Test target updated.Matches the renamed test suite in BUILD.bazel.
.github/workflows/test_scala_no_spark.yaml (1)
54-54: LGTM! Test targets updated consistently.All module test targets follow the new naming convention.
Also applies to: 61-61, 68-68, 75-75, 82-82, 89-89, 96-96
devnotes.md (9)
51-52: New Bazel Setup Section Added.
The new header clearly demarcates the Bazel instructions from the rest.
53-60: Bazel Installation (Mac) Instructions.
Concise commands; consider linking to Bazelisk docs for additional context.
61-66: Bazel Installation (Linux) Instructions.
Clear steps; verify URL/version remains current.
78-82: Remote Caching Instructions.
Straightforward and clear; no issues found.
83-87: Java Not Found Error Fix.
Clear and actionable fix; installs Amazon Corretto-17 as required.
88-100: Uber Jar Build Commands.
Commands are clear; placeholders indicate where to insert module/target values.
102-107: Module Test Command.
Simple and effective instruction for running all tests in a module.
108-112: Individual Test File Command.
Clear example for testing a specific file using placeholders.
114-121: Repository Clean Commands.
Commands are accurate and well explained.
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
devnotes.md (2)
53-67: Bazel Setup: Clear and concise instructions.
The Bazel installation steps for Mac and Linux are well documented. Consider noting that the PATH update on Linux may need to be added permanently to your shell configuration.
83-86: Java Error Fix: Good solution.
The instructions for resolving the Java not found error on Mac are clear. You might add a brief note on setting JAVA_HOME if needed.
… fix (#322) ## Summary Updated dev notes with Bazel installation instructions and java error fix ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Documentation** - Enhanced setup guidance for Bazel installation on both Mac and Linux. - Provided clear instructions for resolving Java-related issues on Mac. - Updated testing procedures by replacing previous instructions with streamlined Bazel commands. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
… fix (#322) ## Summary Updated dev notes with Bazel installation instructions and java error fix ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Documentation** - Enhanced setup guidance for Bazel installation on both Mac and Linux. - Provided clear instructions for resolving Java-related issues on Mac. - Updated testing procedures by replacing previous instructions with streamlined Bazel commands. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
… fix (#322) ## Summary Updated dev notes with Bazel installation instructions and java error fix ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Documentation** - Enhanced setup guidance for Bazel installation on both Mac and Linux. - Provided clear instructions for resolving Java-related issues on Mac. - Updated testing procedures by replacing previous instructions with streamlined Bazel commands. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
… fix (#322) ## Summary Updated dev notes with Bazel installation instructions and java error fix ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Documentation** - Enhanced setup guidance for Bazel installation on both Mac and Linux. - Provided clear instructions for resolving Java-related issues on Mac. - Updated testing procedures by replacing previous instructions with streamlined Bazel commands. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
… fix (#322) ## Summary Updated dev notes with Bazel installation instructions and java error fix ## Cheour clientslist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Documentation** - Enhanced setup guidance for Bazel installation on both Mac and Linux. - Provided clear instructions for resolving Java-related issues on Mac. - Updated testing procedures by replacing previous instructions with streamlined Bazel commands. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
Updated dev notes with Bazel installation instructions and java error fix
Checklist
Summary by CodeRabbit