ci: add some checks in the github action #213
Conversation
WalkthroughThe pull request introduces several changes across multiple GitHub workflow files and a configuration file. The Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Outside diff range and nitpick comments (2)
.github/workflows/e2e.yaml (1)
17-17: LGTM! Consider adding documentation and improving reusability.The repository check is a good security measure. However, consider these improvements:
- Add a comment explaining why this check is necessary
- Consider using a repository allowlist in the workflow inputs or repository variables for better reusability
Example improvement:
runs-on: ubuntu-latest + # Restrict workflow to official repository to prevent unauthorized executions - if: ${{ github.repository == 'GreptimeTeam/helm-charts' }} + if: ${{ contains(fromJson(vars.ALLOWED_REPOSITORIES || '["GreptimeTeam/helm-charts"]'), github.repository) }}.github/workflows/release.yaml (1)
Line range hint
1-94: Consider improving job dependencies and deployment verificationTwo architectural improvements would make the workflow more robust:
- The
release-charts-to-acrjob should likely depend on thereleasejob to ensure proper sequencing.- Replace the hardcoded 5-minute sleep with a more reliable method to verify GitHub Pages deployment, such as polling the pages URL.
Consider this structure for job dependencies:
release-charts-to-acr: needs: [release] # ... rest of the job configFor the GitHub Pages verification, consider implementing a script that polls the pages URL with retries and timeout instead of using a fixed sleep.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
.github/pr-title-checker-config.json(1 hunks).github/workflows/ci.yaml(1 hunks).github/workflows/e2e.yaml(1 hunks).github/workflows/release.yaml(2 hunks)
🔇 Additional comments (3)
.github/pr-title-checker-config.json (1)
7-7: LGTM! Verify team conventions for PR title prefixes.
The addition of "enhancement" as a valid PR title prefix is technically correct. However, since this affects all future PRs, please ensure this aligns with the team's conventional commit message guidelines.
Let's check if there are any documentation files that define the commit message conventions:
.github/workflows/ci.yaml (1)
8-8: Consider different conditions for push vs pull_request events.
While the repository check is good for security, it might block legitimate fork contributions. Consider:
- Allow PR checks from forks but with limited permissions
- Keep strict repository check only for push events
- Add documentation explaining the security rationale
Example improvement:
runs-on: ubuntu-latest
- if: ${{ github.repository == 'GreptimeTeam/helm-charts' }}
+ # Restrict workflow execution based on event type
+ if: |
+ ${{
+ (github.event_name == 'push' && github.repository == 'GreptimeTeam/helm-charts') ||
+ (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository)
+ }}Let's check if there are other workflows that might need similar conditions:
✅ Verification successful
Repository check conditions are consistently implemented across workflows
Based on the verification results, all major workflows (ci.yaml, e2e.yaml, and release.yaml) consistently implement the same repository check condition if: ${{ github.repository == 'GreptimeTeam/helm-charts' }} at the job level. This shows a deliberate security pattern across the codebase.
However, let's verify the event triggers for these workflows to better assess the impact of the suggested changes:
Repository check should be adjusted for CI workflow's PR events
The verification reveals that the CI workflow is unique in accepting both push and pull_request events, while other workflows have specific triggers:
- release.yaml: only push to main
- e2e.yaml: manual workflow_dispatch
- pr-title-checker.yaml: pull_request_target events
The current repository check in ci.yaml would indeed block legitimate PR contributions from forks, while other workflows either don't need changes (e2e, release) or already use appropriate events (pr-title-checker uses pull_request_target).
The original suggestion to differentiate conditions based on event type is valid and necessary specifically for the CI workflow to:
- Maintain strict security for push events
- Allow PR checks from forks
- Keep consistent with the repository's PR handling pattern shown in pr-title-checker.yaml
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all workflow files and check for repository conditions
fd -e yml -e yaml . .github/workflows/ -x sh -c 'echo "=== {} ===" && cat {} | grep -A 2 "if:"'
Length of output: 1344
Script:
#!/bin/bash
# Check workflow triggers
fd -e yml -e yaml . .github/workflows/ -x sh -c 'echo "=== {} ===" && cat {} | grep -A 3 "^on:"'
Length of output: 442
.github/workflows/release.yaml (1)
19-19: LGTM! Repository check adds proper security control.
The condition correctly restricts chart releases to the authorized repository.
Let's verify this workflow hasn't been copied to other repositories:
✅ Verification successful
Repository check is consistently implemented across workflows
The repository condition github.repository == 'GreptimeTeam/helm-charts' is properly implemented across all relevant workflow files (release.yaml, e2e.yaml, and ci.yaml), ensuring a consistent security boundary for all CI/CD operations. This is the correct approach for restricting workflow execution to the authorized repository.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if this workflow exists in other repositories
# Expected: Should only find references in GreptimeTeam/helm-charts
gh search code \
--filename "release.yaml" \
--path ".github/workflows" \
--content "name: Release Charts" \
--json repository \
--jq '.[].repository | select(.nameWithOwner != "GreptimeTeam/helm-charts")'
Length of output: 1029
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to search for similar workflow files
# and repository conditions in the current repository
# Search for workflow files with "Release Charts"
echo "=== Searching for Release Charts workflows ==="
rg -l "name: Release Charts" .github/workflows/
# Search for repository conditions in workflow files
echo -e "\n=== Searching for repository conditions in workflows ==="
rg "github.repository ==" .github/workflows/
Length of output: 726
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
GreptimeTeam/helm-chartsrepository.release-charts-to-acrjob to enhance execution control based on input parameters and events.