Remove automatic execution of test that require LLM inputs from merge… #184
Conversation
… requests onto main
|
Warning Rate limit exceeded@0xideas has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 49 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. WalkthroughThe pull request modifies multiple GitHub Actions workflow files by removing the trigger for pull requests targeting the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GitHub Actions
participant Job Runner
User->>GitHub Actions: Trigger workflow (manual)
GitHub Actions->>Job Runner: Start jobs
Job Runner->>Job Runner: Check for docs changes
alt Changes detected
Job Runner->>Job Runner: Run tests
else No changes
Job Runner->>User: No action taken
end
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: 8
🧹 Outside diff range and nitpick comments (7)
.github/workflows/test_qdrant.yml (1)
1-1: Add workflow documentationConsider adding comments to explain:
- The requirement for the 'run-checks' label
- The purpose of this workflow and its LLM dependency
- The cost implications of running these tests
Add this documentation at the start of the file:
+# This workflow runs Qdrant integration tests that require OpenAI API calls. +# To manage costs, this workflow only runs when: +# 1. Manually triggered via workflow_dispatch +# 2. PR is labeled with 'run-checks' +# 3. Changes are detected outside documentation name: test | qdrant.github/workflows/test_weaviate.yml (1)
1-1: Add workflow documentation and API key safeguardsSince this workflow is being changed to manual execution mode and involves OpenAI API usage, it would be beneficial to:
- Add workflow documentation explaining when and how to trigger these tests
- Consider adding a warning step before using the OpenAI API
Add this documentation at the top of the file:
name: test | weaviate +# This workflow runs Weaviate integration tests that require OpenAI API access. +# To minimize API costs, this workflow must be triggered manually: +# 1. Via workflow_dispatch in GitHub Actions UI +# 2. By adding the 'run-checks' label to your PR (if PR trigger is kept) +# +# Note: This workflow consumes OpenAI credits. Use judiciously..github/workflows/test_neo4j.yml (2)
4-8: Consider removing the branch constraint.Since the workflow now uses label-based control, the branch constraint
branches: [main]is redundant and potentially confusing. The workflow will only run when manually triggered or when PRs are labeled with 'run-checks', regardless of the target branch.workflow_dispatch: pull_request: - branches: - - main types: [labeled]
1-1: Add workflow documentation.Since this workflow contains LLM-dependent tests and now requires manual triggering or specific labels, consider adding documentation at the top of the file to:
- Indicate that this is an LLM-dependent test that consumes OpenAI credits
- Explain how to trigger the workflow (via workflow_dispatch or 'run-checks' label)
name: test | neo4j + +# This workflow runs Neo4j integration tests that require OpenAI API access. +# To prevent unnecessary API costs, this workflow only runs: +# 1. When manually triggered via workflow_dispatch +# 2. When a PR is labeled with 'run-checks'.github/workflows/test_notebook.yml (1)
Notebook credential handling needs improvement for security
The notebook contains potential security concerns in its credential handling:
- Uses environment variables but falls back to empty strings, which is not a secure pattern
- Contains commented-out hardcoded credentials (
DB_PASSWORD)- Direct usage of credentials in Graphistry login call without additional safeguards
Recommended changes:
- Remove the commented-out credential lines (lines with
GRAPH_DATABASE_PASSWORDandDB_PASSWORD)- Replace empty string fallbacks with proper error handling when credentials are missing
- Consider adding credential masking for the Graphistry login call
- Add workflow-level timeout as suggested in the original review
The original security concerns were valid and the verification revealed additional issues that need to be addressed.
🔗 Analysis chain
Line range hint
52-67: Consider additional security measures for notebook execution.While the notebook execution is controlled by the 'run-checks' label, there are some security considerations to address:
- Verify that the notebook doesn't print or expose sensitive credentials
- Consider adding a workflow-level timeout
Consider adding these security enhancements:
- Add a workflow-level timeout:
jobs: run_notebook_test: timeout-minutes: 30 # Adjust based on typical execution time
- Restrict environment variable scope to specific steps that require them
- Add conditions to skip secret injection on fork PRs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential credential exposure in the notebook rg -i "(OPENAI|API_KEY|GRAPHISTRY|PASSWORD|SECRET)" notebooks/cognee_demo.ipynbLength of output: 902
.github/workflows/test_pgvector.yml (2)
4-8: Enhance workflow trigger configuration.While the changes align with the PR objective, consider these improvements:
- Add a description for the workflow_dispatch trigger to clarify its purpose
- Be more specific about which labels trigger the workflow to prevent unintended executions
workflow_dispatch: + description: 'Manually trigger pgvector integration tests' pull_request: branches: - main types: [labeled] + # Ensure the workflow only runs for specific labels + if: | + contains(github.event.pull_request.labels.*.name, 'run-checks') || + contains(github.event.pull_request.labels.*.name, 'integration-test')
1-1: Add documentation about cost implications.Since this workflow uses OpenAI credits, consider adding documentation in the workflow file to:
- Indicate that this is a cost-intensive test
- Explain when the test should be manually triggered
- Document the expected costs per run
name: test | pgvector +# WARNING: This workflow uses OpenAI API and incurs costs +# Manual triggers should be used judiciously +# Estimated cost per run: [Add cost estimate] +# When to run: +# - Before merging significant changes to vector store functionality +# - When testing LLM integration changes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.github/workflows/test_neo4j.yml(2 hunks).github/workflows/test_notebook.yml(2 hunks).github/workflows/test_pgvector.yml(2 hunks).github/workflows/test_qdrant.yml(2 hunks).github/workflows/test_weaviate.yml(2 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/test_neo4j.yml
18-18: "steps" section is missing in job "if"
(syntax-check)
18-18: "runs-on" section is missing in job "if"
(syntax-check)
18-18: "if" job is scalar node but mapping node is expected
(syntax-check)
.github/workflows/test_notebook.yml
19-19: "steps" section is missing in job "if"
(syntax-check)
19-19: "runs-on" section is missing in job "if"
(syntax-check)
19-19: "if" job is scalar node but mapping node is expected
(syntax-check)
.github/workflows/test_pgvector.yml
19-19: "steps" section is missing in job "if"
(syntax-check)
19-19: "runs-on" section is missing in job "if"
(syntax-check)
19-19: "if" job is scalar node but mapping node is expected
(syntax-check)
.github/workflows/test_qdrant.yml
19-19: "steps" section is missing in job "if"
(syntax-check)
19-19: "runs-on" section is missing in job "if"
(syntax-check)
19-19: "if" job is scalar node but mapping node is expected
(syntax-check)
.github/workflows/test_weaviate.yml
19-19: "steps" section is missing in job "if"
(syntax-check)
19-19: "runs-on" section is missing in job "if"
(syntax-check)
19-19: "if" job is scalar node but mapping node is expected
(syntax-check)
🔇 Additional comments (1)
.github/workflows/test_qdrant.yml (1)
Line range hint 52-54: Verify secure handling of API keys
The workflow uses sensitive API keys. Let's ensure they're consistently handled securely across the codebase.
| workflow_dispatch: | ||
| pull_request: | ||
| branches: | ||
| - main | ||
| workflow_dispatch: | ||
| types: [labeled] |
There was a problem hiding this comment.
Remove pull_request trigger for main branch to align with PR objectives
The current configuration still allows automatic execution on pull requests to main, which contradicts the PR's goal of removing automatic execution of LLM-dependent tests.
Apply this diff to fix:
on:
workflow_dispatch:
pull_request:
- branches:
- - main
types: [labeled]📝 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.
| workflow_dispatch: | |
| pull_request: | |
| branches: | |
| - main | |
| workflow_dispatch: | |
| types: [labeled] | |
| workflow_dispatch: | |
| pull_request: | |
| types: [labeled] |
.github/workflows/test_qdrant.yml
Outdated
| if: ${{ github.event.label.name == 'run-checks' }} | ||
|
|
There was a problem hiding this comment.
Fix incorrect YAML structure for job conditions
The conditional check is incorrectly placed as a separate job. It should be moved to the individual jobs that need this condition.
Apply this diff to fix:
jobs:
- if: ${{ github.event.label.name == 'run-checks' }}
get_docs_changes:
name: docs changes
uses: ./.github/workflows/get_docs_changes.yml
run_qdrant_integration_test:
name: test
needs: get_docs_changes
- if: needs.get_docs_changes.outputs.changes_outside_docs == 'true'
+ if: |
+ needs.get_docs_changes.outputs.changes_outside_docs == 'true' &&
+ github.event.label.name == 'run-checks'
runs-on: ubuntu-latest📝 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.
| if: ${{ github.event.label.name == 'run-checks' }} | |
| jobs: | |
| get_docs_changes: | |
| name: docs changes | |
| uses: ./.github/workflows/get_docs_changes.yml | |
| run_qdrant_integration_test: | |
| name: test | |
| needs: get_docs_changes | |
| if: | | |
| needs.get_docs_changes.outputs.changes_outside_docs == 'true' && | |
| github.event.label.name == 'run-checks' | |
| runs-on: ubuntu-latest |
🧰 Tools
🪛 actionlint
19-19: "steps" section is missing in job "if"
(syntax-check)
19-19: "runs-on" section is missing in job "if"
(syntax-check)
19-19: "if" job is scalar node but mapping node is expected
(syntax-check)
| workflow_dispatch: | ||
| pull_request: | ||
| branches: | ||
| - main | ||
| workflow_dispatch: | ||
| types: [labeled] | ||
|
|
There was a problem hiding this comment.
Workflow still triggers automatically on PRs to main branch
The current configuration contradicts the PR objective of removing automatic test execution on merge requests to main. While the label condition was added, the workflow will still trigger automatically when PRs are labeled.
To align with the PR objective, modify the trigger configuration:
on:
workflow_dispatch:
- pull_request:
- branches:
- - main
- types: [labeled]This ensures the workflow only runs when manually triggered via workflow_dispatch.
📝 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.
| workflow_dispatch: | |
| pull_request: | |
| branches: | |
| - main | |
| workflow_dispatch: | |
| types: [labeled] | |
| workflow_dispatch: |
.github/workflows/test_weaviate.yml
Outdated
| if: ${{ github.event.label.name == 'run-checks' }} | ||
|
|
There was a problem hiding this comment.
Fix YAML structure: Incorrect placement of if condition
The label condition is incorrectly placed as a separate job. This will cause the workflow to fail.
Move the condition to each job that needs it:
jobs:
- if: ${{ github.event.label.name == 'run-checks' }}
get_docs_changes:
name: docs changes
+ if: ${{ github.event.label.name == 'run-checks' }}
uses: ./.github/workflows/get_docs_changes.yml
run_weaviate_integration_test:
name: test
needs: get_docs_changes
- if: needs.get_docs_changes.outputs.changes_outside_docs == 'true'
+ if: |
+ needs.get_docs_changes.outputs.changes_outside_docs == 'true' &&
+ github.event.label.name == 'run-checks'📝 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.
| if: ${{ github.event.label.name == 'run-checks' }} | |
| get_docs_changes: | |
| name: docs changes | |
| if: ${{ github.event.label.name == 'run-checks' }} | |
| uses: ./.github/workflows/get_docs_changes.yml | |
| run_weaviate_integration_test: | |
| name: test | |
| needs: get_docs_changes | |
| if: | | |
| needs.get_docs_changes.outputs.changes_outside_docs == 'true' && | |
| github.event.label.name == 'run-checks' |
🧰 Tools
🪛 actionlint
19-19: "steps" section is missing in job "if"
(syntax-check)
19-19: "runs-on" section is missing in job "if"
(syntax-check)
19-19: "if" job is scalar node but mapping node is expected
(syntax-check)
.github/workflows/test_neo4j.yml
Outdated
| if: ${{ github.event.label.name == 'run-checks' }} | ||
|
|
There was a problem hiding this comment.
Fix invalid YAML structure.
The current placement of the if condition is incorrect and will cause the workflow to fail. The condition should be moved into each job that needs to be controlled by the label.
Apply this change to fix the YAML structure:
jobs:
- if: ${{ github.event.label.name == 'run-checks' }}
get_docs_changes:
name: docs changes
uses: ./.github/workflows/get_docs_changes.yml
+ if: ${{ github.event.label.name == 'run-checks' }}
run_neo4j_integration_test:
name: test
needs: get_docs_changes
- if: needs.get_docs_changes.outputs.changes_outside_docs == 'true'
+ if: |
+ needs.get_docs_changes.outputs.changes_outside_docs == 'true' &&
+ github.event.label.name == 'run-checks'
runs-on: ubuntu-latest📝 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.
| if: ${{ github.event.label.name == 'run-checks' }} | |
| get_docs_changes: | |
| name: docs changes | |
| uses: ./.github/workflows/get_docs_changes.yml | |
| if: ${{ github.event.label.name == 'run-checks' }} | |
| run_neo4j_integration_test: | |
| name: test | |
| needs: get_docs_changes | |
| if: | | |
| needs.get_docs_changes.outputs.changes_outside_docs == 'true' && | |
| github.event.label.name == 'run-checks' | |
| runs-on: ubuntu-latest |
🧰 Tools
🪛 actionlint
18-18: "steps" section is missing in job "if"
(syntax-check)
18-18: "runs-on" section is missing in job "if"
(syntax-check)
18-18: "if" job is scalar node but mapping node is expected
(syntax-check)
.github/workflows/test_notebook.yml
Outdated
| if: ${{ github.event.label.name == 'run-checks' }} | ||
|
|
There was a problem hiding this comment.
Fix YAML structure for conditional execution.
The current job-level condition is incorrectly structured and will cause syntax errors.
Apply this diff to fix the YAML structure:
jobs:
- if: ${{ github.event.label.name == 'run-checks' }}
-
get_docs_changes:
name: docs changes
+ if: github.event.label.name == 'run-checks' || github.event_name == 'workflow_dispatch'
uses: ./.github/workflows/get_docs_changes.yml
run_notebook_test:
name: test
needs: get_docs_changes
- if: needs.get_docs_changes.outputs.changes_outside_docs == 'true'
+ if: (github.event.label.name == 'run-checks' || github.event_name == 'workflow_dispatch') && needs.get_docs_changes.outputs.changes_outside_docs == 'true'This change:
- Fixes the YAML syntax error
- Applies the label condition correctly to each job
- Preserves the existing docs changes check
- Allows execution via manual dispatch
📝 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.
| if: ${{ github.event.label.name == 'run-checks' }} | |
| jobs: | |
| get_docs_changes: | |
| name: docs changes | |
| if: github.event.label.name == 'run-checks' || github.event_name == 'workflow_dispatch' | |
| uses: ./.github/workflows/get_docs_changes.yml | |
| run_notebook_test: | |
| name: test | |
| needs: get_docs_changes | |
| if: (github.event.label.name == 'run-checks' || github.event_name == 'workflow_dispatch') && needs.get_docs_changes.outputs.changes_outside_docs == 'true' |
🧰 Tools
🪛 actionlint
19-19: "steps" section is missing in job "if"
(syntax-check)
19-19: "runs-on" section is missing in job "if"
(syntax-check)
19-19: "if" job is scalar node but mapping node is expected
(syntax-check)
| workflow_dispatch: | ||
| pull_request: | ||
| branches: | ||
| - main | ||
| workflow_dispatch: | ||
| types: [labeled] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance trigger conditions to prevent unnecessary workflow runs.
While the changes align with the goal of preventing automatic test execution, the current trigger configuration could be improved to be more precise.
Apply this diff to handle both label addition and removal, and specifically filter for the 'run-checks' label:
workflow_dispatch:
pull_request:
branches:
- main
- types: [labeled]
+ types: [labeled, unlabeled]This ensures the workflow:
- Can be triggered manually when needed
- Runs when the 'run-checks' label is added
- Stops when the label is removed
Committable suggestion skipped: line range outside the PR's diff.
.github/workflows/test_pgvector.yml
Outdated
| if: ${{ github.event.label.name == 'run-checks' }} | ||
|
|
There was a problem hiding this comment.
Fix critical YAML syntax error in job conditions.
The if condition is incorrectly placed at the jobs level, causing syntax errors. It should be moved into each individual job.
jobs:
- if: ${{ github.event.label.name == 'run-checks' }}
get_docs_changes:
name: docs changes
uses: ./.github/workflows/get_docs_changes.yml
+ if: ${{ github.event.label.name == 'run-checks' }}
run_pgvector_integration_test:
name: test
needs: get_docs_changes
- if: needs.get_docs_changes.outputs.changes_outside_docs == 'true'
+ if: |
+ needs.get_docs_changes.outputs.changes_outside_docs == 'true' &&
+ github.event.label.name == 'run-checks'📝 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.
| if: ${{ github.event.label.name == 'run-checks' }} | |
| jobs: | |
| get_docs_changes: | |
| name: docs changes | |
| uses: ./.github/workflows/get_docs_changes.yml | |
| if: ${{ github.event.label.name == 'run-checks' }} | |
| run_pgvector_integration_test: | |
| name: test | |
| needs: get_docs_changes | |
| if: | | |
| needs.get_docs_changes.outputs.changes_outside_docs == 'true' && | |
| github.event.label.name == 'run-checks' |
🧰 Tools
🪛 actionlint
19-19: "steps" section is missing in job "if"
(syntax-check)
19-19: "runs-on" section is missing in job "if"
(syntax-check)
19-19: "if" job is scalar node but mapping node is expected
(syntax-check)
|
Hey @Vasilije1990, the new workflow would be to add the label 'run-checks' to trigger the LLM-intensive tests before merging, and to allow merging only if this label is present. This isn't an ideal solution but the only alternative is an additional 'meta' GitHub action that manages the triggering of these tests. The solution implemented here is also the one suggested here |
0xideas
force-pushed
the
COG-531-trigger-checks-manually
branch
from
935c784 to
236a8c1
Compare
… requests onto main.
To save money on OpenAI credits.
Summary by CodeRabbit
New Features
Bug Fixes
mainbranch across multiple workflows, ensuring more controlled execution.Improvements
run-checks, allowing for conditional execution based on specific criteria.