build(ci): optional jobs must report success to merge PR #993
Conversation
|
Warning Rate limit exceeded@lklimek has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes in the workflow configuration files enhance the checks for generated code by removing the Changes
Possibly related PRs
Suggested reviewers
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (2)
.github/workflows/pr-precheck.yml (2)
4-10: Consider enhancing the workflow trigger configuration.The current trigger configuration could be improved in several ways:
- Consider adding branch filters to limit execution
- Add handling for when the trigger workflow is skipped
- Consider security implications of
workflow_runeventAdd branch filters and additional workflow triggers:
on: workflow_run: workflows: - "Check generated code" + # Add other optional job workflows here + branches: + - main + - 'v[0-9]+.x' # version branches types: - completed
12-24: Consider adding operational robustness features.To improve the reliability and observability of the workflow:
- Add retry mechanism for transient failures
- Configure appropriate timeouts
- Implement notification mechanism for persistent failures
Consider adding these configurations:
jobs: wait-for-optional-jobs: runs-on: ubuntu-latest + timeout-minutes: 10 + strategy: + retry: + max-attempts: 3 + wait-duration: 60s steps: + - name: Configure notifications + uses: actions/github-script@v6 + with: + script: | + const channel = '#ci-alerts'; + const message = `Optional jobs failed in ${process.env.GITHUB_REPOSITORY}`; + // Add notification logic here
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pr-precheck.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/pr-precheck.yml (1)
12-24: 🛠️ Refactor suggestion
Enhance error reporting and job status handling.
The current implementation lacks detailed error reporting and robust status handling:
- Error messages don't specify which jobs failed
- No detailed status information is collected
- No artifact or summary creation for failures
Consider this improved implementation:
jobs:
wait-for-optional-jobs:
runs-on: ubuntu-latest
steps:
- - name: Check status of optional jobs
+ - name: Check detailed status of optional jobs
shell: bash
run: |
- if [[ "${{ github.event.workflow_run.conclusion }}" == "success" || "${{ github.event.workflow_run.conclusion }}" == "skipped" ]]; then
- echo "All optional jobs passed or were skipped."
- else
- echo "Some optional jobs failed."
- exit 1
- fi
+ # Get detailed job status using GitHub API
+ JOBS_STATUS=$(gh api \
+ "/repos/${{ github.repository }}/actions/runs/${{ github.event.workflow_run.id }}/jobs" \
+ --jq '.jobs[] | select(.conclusion != "success" and .conclusion != "skipped") | .name')
+
+ if [ ! -z "$JOBS_STATUS" ]; then
+ echo "::error::The following optional jobs failed:"
+ echo "$JOBS_STATUS" | while read job; do
+ echo "::error:: - $job"
+ done
+ exit 1
+ else
+ echo "::notice::All optional jobs completed successfully or were skipped"
+ fi
+ env:
+ GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}This implementation:
- Uses GitHub API to get detailed job status
- Provides specific information about failed jobs
- Uses workflow commands for better error visibility
- Properly handles authentication
Let's verify the GitHub API endpoint availability:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
.github/workflows/check-generated.yml (3)
29-32: Consider pinning the diff action to a specific SHA.While using
@v6is acceptable, pinning to a specific SHA would provide better security and reproducibility.- - uses: technote-space/get-diff-action@v6 + - uses: technote-space/[email protected]
35-35: Document the mockery version requirement.Consider adding a comment explaining why this specific version (2.49.1) of mockery is required, or move it to a central version management location.
Line range hint
29-69: Well-designed conditional execution strategy.The addition of diff detection and conditional execution is a good architectural choice that:
- Improves CI efficiency by skipping unnecessary checks
- Maintains reliability by still running when relevant files change
- Provides clear error messages for maintainability
The implementation is consistent between mocks and proto checks, which is good for maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/check-generated.yml(2 hunks)
🔇 Additional comments (3)
.github/workflows/check-generated.yml (3)
26-28: LGTM! Optimal checkout configuration.
The minimal fetch depth is correctly set for git diff operations while optimizing checkout speed.
63-67: Same version pinning consideration applies here.
69-69: 🛠️ Refactor suggestion
Pin the gogo protobuf version.
Using @latest for gogo protobuf could lead to inconsistent builds. Consider pinning to a specific version.
- go install github.com/gogo/protobuf/protoc-gen-gogofaster@latest
+ go install github.com/gogo/protobuf/[email protected]Likely invalid or redundant comment.
lklimek
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
.github/workflows/tests.yml (1)
Line range hint
47-53: Consider adding explicit success step when no changes detectedWhile the workflow will pass when GIT_DIFF is empty (as no steps will run), it might be helpful to add an explicit step that runs when there are no relevant changes to provide clearer feedback in the GitHub UI.
- name: Run Go Tests if: env.GIT_DIFF env: CGO_LDFLAGS: "-L/usr/local/lib -ldashbls -lrelic_s -lmimalloc-secure -lgmp" CGO_CXXFLAGS: "-I/usr/local/include" run: | make test-group-${{ matrix.part }} NUM_SPLIT=6 + + - name: Skip tests - no relevant changes + if: "!env.GIT_DIFF" + run: echo "No Go files or dependencies were modified - skipping tests".github/workflows/lint.yml (1)
Line range hint
54-63: Consider adding explicit success step for skipped lintingSimilar to the tests workflow, consider adding an explicit step to provide clear feedback when linting is skipped due to no relevant changes.
- uses: golangci/[email protected] if: env.GIT_DIFF with: version: v1.61 args: --timeout 10m github-token: ${{ secrets.github_token }} only-new-issues: true + + - name: Skip linting - no relevant changes + if: "!env.GIT_DIFF" + run: echo "No Go files or linter configurations were modified - skipping linting".github/workflows/e2e.yml (2)
Line range hint
46-90: Refactor repeated condition into job-level conditionThe condition
"github.event_name != 'pull_request' || env.GIT_DIFF != ''"is repeated multiple times. Consider moving it to the job level using a job condition.e2e-test: runs-on: ubuntu-latest + if: "github.event_name != 'pull_request' || env.GIT_DIFF != ''" strategy: fail-fast: true matrix: testnet: ["dashcore", "rotate"] steps: # ... other steps ... - uses: ./.github/actions/bls name: Install BLS library - if: "github.event_name != 'pull_request' || env.GIT_DIFF != ''" - name: Build runner and tests working-directory: test/e2e run: make -j2 runner tests - if: "github.event_name != 'pull_request' || env.GIT_DIFF != ''" # ... other steps with the same condition ...Also consider adding an explicit success message when skipped:
+ - name: Skip E2E tests - no relevant changes + if: "github.event_name == 'pull_request' && !env.GIT_DIFF" + run: echo "No Go files, Dockerfile, or dependencies were modified - skipping E2E tests"
Line range hint
1-90: Architecture Review: Consistent and effective implementationThe implementation effectively solves the problem of optional jobs not reporting success when no relevant files are changed. The approach using
technote-space/get-diff-actionis consistent across all workflows and provides a clean solution to:
- Detect relevant file changes
- Skip unnecessary workflow steps
- Report success when no relevant changes are found
While the current implementation works, consider the following architectural improvements:
- Create a reusable workflow for the common get-diff-action setup
- Add explicit success messages for better visibility
- Use job-level conditions where possible to reduce repetition
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.github/workflows/e2e.yml(1 hunks).github/workflows/lint.yml(1 hunks).github/workflows/tests.yml(1 hunks)proto/tendermint/abci/types.proto(1 hunks)test/e2e/node/main.go(1 hunks)
🔇 Additional comments (5)
.github/workflows/tests.yml (1)
18-28: LGTM: Appropriate file pattern configuration
The get-diff-action configuration correctly identifies relevant Go files while excluding test files, and includes essential build configuration files.
.github/workflows/lint.yml (1)
29-39: LGTM: Comprehensive linting file pattern configuration
The get-diff-action configuration appropriately includes:
- Go source files
- Linter configurations
- Go module files
.github/workflows/e2e.yml (1)
38-44: LGTM: Appropriate file pattern configuration for E2E tests
The get-diff-action configuration correctly includes:
- Dockerfile changes
- Go source files
- Go module files
test/e2e/node/main.go (1)
108-108: LGTM! Minor comment formatting change.
The removal of the period from the comment is a minor cosmetic change that doesn't affect functionality.
proto/tendermint/abci/types.proto (1)
17-17: LGTM! Important documentation addition.
The added comment provides a crucial reference to protobuf custom types documentation, which helps developers avoid common pitfalls when implementing custom types. This is particularly important for maintaining protocol compatibility and avoiding serialization issues.
Issue being fixed or feature implemented
When some Github Actions checks are marked as mandatory checks, but they don't get invoked (eg. the PR didn't modify files that trigger the check), they never pass and we need to force merge.
What was done?
Instead of adding
on: pull_request: paths:, we usetechnote-space/get-diff-action@v6to always run the workflow, and fail after first few steps.Some disadvantage is that these tests are marked "green". Unfortunately, GHA does not implement
neutraloutcome, see actions/runner#2347.How Has This Been Tested?
GHA
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Summary by CodeRabbit