-
-
Notifications
You must be signed in to change notification settings - Fork 864
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
NoMongo: Restore the GraphQL Introspection GitHub Action Test #3496
base: develop-postgres
Are you sure you want to change the base?
NoMongo: Restore the GraphQL Introspection GitHub Action Test #3496
Conversation
WalkthroughThe pull request modifies the GitHub Actions workflow in Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
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
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
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 (1)
.github/workflows/pull-request.yml (1)
276-302
: Optimize the GraphQL Inspector job configuration.The job configuration can be improved in several ways:
- Add conditional execution to run only when GraphQL files change
- Fix trailing spaces in the script
- Consider caching the graphql-inspector CLI installation
Apply these improvements:
Graphql-Inspector: - # if: ${{ github.actor != 'dependabot[bot]' }} + if: | + github.actor != 'dependabot[bot]' && + contains(github.event.pull_request.files.*.filename, 'GraphQl') || + contains(github.event.pull_request.files.*.filename, 'schema.graphql') name: Runs Introspection on the GitHub talawa-api repo on the schema.graphql file runs-on: ubuntu-latest steps: - name: Checkout the Repository uses: actions/checkout@v4 - name: Set up Node.js uses: actions/setup-node@v4 with: node-version: '22.x' + cache: 'npm' - name: resolve dependency - run: npm install -g @graphql-inspector/cli + run: npm install -g @graphql-inspector/cli - name: Clone API Repository run: | # Retrieve the complete branch name directly from the GitHub context FULL_BRANCH_NAME=${{ github.base_ref }} - echo "FULL_Branch_NAME: $FULL_BRANCH_NAME" + echo "FULL_BRANCH_NAME: $FULL_BRANCH_NAME" # Clone the specified repository using the extracted branch name - git clone --branch $FULL_BRANCH_NAME https://github.com/PalisadoesFoundation/talawa-api && ls -a + git clone --branch $FULL_BRANCH_NAME https://github.com/PalisadoesFoundation/talawa-api && ls -a - name: Validate Documents run: graphql-inspector validate './src/GraphQl/**/*.ts' './talawa-api/schema.graphql'🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 291-291: trailing spaces
(trailing-spaces)
[error] 297-297: trailing spaces
(trailing-spaces)
[error] 299-299: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml
(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/pull-request.yml
[error] 291-291: trailing spaces
(trailing-spaces)
[error] 297-297: trailing spaces
(trailing-spaces)
[error] 299-299: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test Application
- GitHub Check: Check Python Code Style
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (1)
.github/workflows/pull-request.yml (1)
41-41
: Verify the necessity of increasing the line count threshold.The line count threshold has been significantly increased from 600 to 1100 lines. This substantial increase could indicate growing complexity in the codebase.
Run this script to analyze the line count distribution across files:
✅ Verification successful
The line count threshold increase is justified by the codebase metrics.
The increase from 600 to 1100 lines is supported by the actual file sizes in the codebase, particularly test files, mock data, and GraphQL query files that naturally tend to be larger. The excluded files list appropriately handles known large files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze line count distribution to justify the threshold increase # Find all TypeScript/JavaScript files and count their lines echo "=== Line count distribution ===" for file in $(find . -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.js" -o -name "*.jsx" \) -not -path "*/node_modules/*"); do lines=$(wc -l < "$file") echo "$lines $file" done | sort -nr | head -n 10 echo -e "\n=== Files exceeding old threshold (600) ===" for file in $(find . -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.js" -o -name "*.jsx" \) -not -path "*/node_modules/*"); do lines=$(wc -l < "$file") if [ "$lines" -gt 600 ]; then echo "$lines $file" fi doneLength of output: 66173
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3496 +/- ##
=================================================
Coverage 88.65% 88.65%
=================================================
Files 341 341
Lines 8626 8626
Branches 1925 1925
=================================================
Hits 7647 7647
Misses 635 635
Partials 344 344 ☔ View full report in Codecov by Sentry. |
6051ed6
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: 1
🧹 Nitpick comments (2)
.github/workflows/pull-request.yml (2)
304-306
: Simplify branch name handling.The current implementation includes unnecessary echo statements and variable assignments.
- # Retrieve the complete branch name directly from the GitHub context - FULL_BRANCH_NAME="$BASE_REF" - echo "FULL_BRANCH_NAME: $FULL_BRANCH_NAME" + # Use BASE_REF directly
330-334
: Improve GraphQL files search pattern.The current implementation searches for all .ts files in the GraphQL directory. Consider making the search more specific to GraphQL-related TypeScript files.
- GRAPHQL_FILES=$(find ./src/GraphQl -name "*.ts" -type f) + GRAPHQL_FILES=$(find ./src/GraphQl -name "*{Query,Mutation,Fragment,Subscription}.ts" -type f)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml
(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/pull-request.yml
[error] 275-275: trailing spaces
(trailing-spaces)
[error] 299-299: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test Application
- GitHub Check: Check Python Code Style
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (2)
.github/workflows/pull-request.yml (2)
276-282
: LGTM! Well-implemented GraphQL schema validation.The implementation includes proper error handling and uses graphql-inspector for schema validation.
41-41
: Verify the necessity of increasing the line count threshold.The line count threshold has been significantly increased from 600 to 1100 lines. While this allows the workflow to process larger files, it might impact code maintainability.
✅ Verification successful
The increased line count threshold is justified
The increase from 600 to 1100 lines is appropriate as 9 out of 10 excluded files already exceed the old threshold, with several files containing 800-900 lines of code. The new limit provides a reasonable buffer for these files while still maintaining a cap on file size.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if any files are close to or exceed the previous threshold # to understand if this increase is necessary. # Find TypeScript/JavaScript files and count their lines find . -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.js" -o -name "*.jsx" \) -not -path "*/node_modules/*" -not -path "*/dist/*" -exec wc -l {} \; | sort -nr | head -n 5Length of output: 508
Script:
#!/bin/bash # Check line count of specifically excluded files for file in "src/screens/LoginPage/LoginPage.tsx" "src/GraphQl/Queries/Queries.ts" "src/screens/OrgList/OrgList.tsx" "src/GraphQl/Mutations/mutations.ts" "src/components/EventListCard/EventListCardModals.tsx" "src/components/TagActions/TagActionsMocks.ts" "src/utils/interfaces.ts" "src/screens/MemberDetail/MemberDetail.tsx" "src/components/OrgPostCard/OrgPostCard.tsx" "src/components/UsersTableItem/UsersTableItem.tsx"; do if [ -f "$file" ]; then wc -l "$file" else echo "File not found: $file" fi doneLength of output: 5659
e285b4f
to
22369eb
Compare
7b6856a
22369eb
to
7b6856a
Compare
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)
.github/workflows/pull-request.yml (2)
297-298
: Pin the @graphql-inspector/cli version.Using
latest
tag can lead to unexpected breaking changes. Consider pinning to a specific version for better reproducibility.- run: npm install -g @graphql-inspector/cli@latest + run: npm install -g @graphql-inspector/[email protected]
337-337
: Consider using a more specific path pattern.The current pattern might include unnecessary files. Consider being more specific about the GraphQL operation types.
- npx graphql-inspector validate './src/GraphQl/**/*.{ts,tsx}' './talawa-api/schema.graphql' + npx graphql-inspector validate './src/GraphQl/{Queries,Mutations,Subscriptions}/**/*.{ts,tsx}' './talawa-api/schema.graphql'
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml
(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/pull-request.yml
[error] 275-275: trailing spaces
(trailing-spaces)
[error] 299-299: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Check Python Code Style
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (3)
.github/workflows/pull-request.yml (3)
41-41
: Justify the significant increase in line count threshold.The line count threshold has been increased from 600 to 1100 lines, which is a significant 83% increase. Please provide justification for this change as it could potentially mask code quality issues.
276-282
: LGTM! Robust GraphQL schema validation.The implementation includes proper error handling and uses graphql-inspector for comprehensive schema validation.
284-338
: LGTM! Well-implemented GraphQL introspection job.The job includes:
- Proper error handling for repository cloning
- Clear error messages
- Branch validation
- File existence checks
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 299-299: trailing spaces
(trailing-spaces)
e2d0e5d
7b6856a
to
e2d0e5d
Compare
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.
- Reset your edit to the countline.py script. It is not in scope of the PR.
- Please don't edit code that is not relevant to the issue
e2d0e5d
to
74ff504
Compare
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: 1
🧹 Nitpick comments (2)
.github/workflows/pull-request.yml (2)
275-275
: Remove trailing spaces.Remove trailing spaces from lines 275 and 299.
Also applies to: 299-299
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 275-275: trailing spaces
(trailing-spaces)
284-338
: Optimize Graphql-Inspector job performance and error handling.The job implementation is solid but could benefit from caching and improved error messages.
Graphql-Inspector: if: ${{ github.actor != 'dependabot[bot]' }} name: Runs Introspection on the GitHub talawa-api repo on the schema.graphql file runs-on: ubuntu-latest steps: - name: Checkout the Repository uses: actions/checkout@v4 - name: Set up Node.js uses: actions/setup-node@v4 with: node-version: '22.x' + cache: 'npm' - name: resolve dependency run: npm install -g @graphql-inspector/cli@latest - name: Clone API Repository env: BASE_REF: ${{ github.event.pull_request.base.ref }} run: | # Retrieve the complete branch name directly from the GitHub context FULL_BRANCH_NAME="$BASE_REF" echo "FULL_BRANCH_NAME: $FULL_BRANCH_NAME" # Clone the specified repository using the extracted branch name if ! git ls-remote --heads https://github.com/PalisadoesFoundation/talawa-api.git $FULL_BRANCH_NAME | grep -q .; then echo "Error: Branch '$FULL_BRANCH_NAME' does not exist in talawa-api repository" echo "Please ensure the branch exists and you have the correct permissions" exit 1 fi if ! git clone --branch $FULL_BRANCH_NAME --depth 1 https://github.com/PalisadoesFoundation/talawa-api; then echo "Error: Failed to clone talawa-api repository from branch '$FULL_BRANCH_NAME'" echo "This could be due to network issues or repository permissions" exit 1 fi # Verify clone was successful if [ ! -d "talawa-api" ]; then echo "Error: talawa-api directory not found after clone" exit 1 fi - name: Validate Documents run: | if [ ! -f './talawa-api/schema.graphql' ]; then echo "Error: schema.graphql not found in talawa-api" exit 1 fi GRAPHQL_FILES=$(find ./src/GraphQl -type f \( -name "*.ts" -o -name "*.tsx" \)) if [ -z "$GRAPHQL_FILES" ]; then echo "Error: No GraphQL TypeScript files found" exit 1 fi echo "Found GraphQL files:" echo "$GRAPHQL_FILES" - graphql-inspector validate './src/GraphQl/**/*.{ts,tsx}' './talawa-api/schema.graphql' + # Run validation with detailed error reporting + if ! graphql-inspector validate './src/GraphQl/**/*.{ts,tsx}' './talawa-api/schema.graphql'; then + echo "Error: GraphQL validation failed. Please check if:" + echo "1. All GraphQL operations match the schema" + echo "2. All required fields are included" + echo "3. No deprecated fields are being used" + exit 1 + fi🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 299-299: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml
(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/pull-request.yml
[error] 275-275: trailing spaces
(trailing-spaces)
[error] 299-299: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
Sorry for that change. Yesterday, I encountered a check failure because some files exceeded the 600-line limit. |
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: 2
🧹 Nitpick comments (3)
.github/workflows/pull-request.yml (3)
301-324
: Consider caching the API repository.The implementation has good error handling but could benefit from caching the cloned repository to improve workflow performance.
Add caching using GitHub's cache action:
+ - name: Cache API Repository + id: cache-api + uses: actions/cache@v4 + with: + path: talawa-api + key: talawa-api-${{ github.event.pull_request.base.ref }}-${{ hashFiles('talawa-api/schema.graphql') }} + - name: Clone API Repository + if: steps.cache-api.outputs.cache-hit != 'true' env: BASE_REF: ${{ github.event.pull_request.base.ref }} run: |
325-338
: Enhance error reporting in GraphQL validation.While the implementation is solid, it could provide more detailed error output to help developers quickly identify and fix issues.
Apply this diff to improve error reporting:
- name: Validate Documents run: | if [ ! -f './talawa-api/schema.graphql' ]; then echo "Error: schema.graphql not found in talawa-api" + echo "Expected location: ./talawa-api/schema.graphql" + echo "Please ensure the file exists and has the correct permissions" exit 1 fi GRAPHQL_FILES=$(find ./src/GraphQl -type f \( -name "*.ts" -o -name "*.tsx" \)) if [ -z "$GRAPHQL_FILES" ]; then echo "Error: No GraphQL TypeScript files found" + echo "Expected location: ./src/GraphQl/**/*.{ts,tsx}" + echo "Please ensure your GraphQL files are in the correct directory" exit 1 fi echo "Found GraphQL files:" echo "$GRAPHQL_FILES" + echo "Validating GraphQL documents against schema..." graphql-inspector validate './src/GraphQl/**/*.{ts,tsx}' './talawa-api/schema.graphql' + echo "GraphQL validation completed successfully!"
275-275
: Remove trailing spaces.There are trailing spaces on lines 275, 284, and 300 that should be removed for consistency.
Apply this diff:
- + - + - +Also applies to: 284-284, 300-300
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 275-275: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml
(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/pull-request.yml
[error] 275-275: trailing spaces
(trailing-spaces)
[error] 284-284: trailing spaces
(trailing-spaces)
[error] 300-300: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test Application
- GitHub Check: Check Python Code Style
- GitHub Check: Analyse Code With CodeQL (javascript)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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)
.github/workflows/pull-request.yml (3)
276-291
: Enhance error handling for GraphQL schema validation.The validation step should handle validation failures explicitly.
- name: Validate GraphQL schema run: | # Install graphql-inspector if not already installed npm install -g @graphql-inspector/cli@latest # Check if any .graphql files exist if ! find src -name "*.graphql" -type f | grep -q .; then echo "Warning: No .graphql files found in src directory" exit 0 fi # Check if schema.graphql exists if [ ! -f "schema.graphql" ]; then echo "Error: schema.graphql not found in the root directory" exit 1 fi - npx graphql-inspector validate schema.graphql "src/**/*.graphql" + if ! npx graphql-inspector validate schema.graphql "src/**/*.graphql"; then + echo "Error: GraphQL schema validation failed" + exit 1 + fi🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized modification or deletion of sensitive file. This file requires special permissions. To proceed, apply the 'ignore-sensitive-files-pr' label.
308-331
: Add timeout and cleanup for git operations.While the error handling is robust, consider adding:
- Timeout for git operations to prevent hanging
- Cleanup of cloned repository in case of failure
- name: Clone API Repository env: BASE_REF: ${{ github.event.pull_request.base.ref }} run: | + # Set timeout for git operations + export GIT_HTTP_LOW_SPEED_LIMIT=1000 + export GIT_HTTP_LOW_SPEED_TIME=10 + # Retrieve the complete branch name directly from the GitHub context FULL_BRANCH_NAME="$BASE_REF" echo "FULL_BRANCH_NAME: $FULL_BRANCH_NAME" # Clone the specified repository using the extracted branch name if ! git ls-remote --heads https://github.com/PalisadoesFoundation/talawa-api.git $FULL_BRANCH_NAME | grep -q .; then echo "Error: Branch '$FULL_BRANCH_NAME' does not exist in talawa-api repository" echo "Please ensure the branch exists and you have the correct permissions" exit 1 fi + # Cleanup any existing clone + rm -rf talawa-api || true if ! git clone --branch $FULL_BRANCH_NAME --depth 1 https://github.com/PalisadoesFoundation/talawa-api; then echo "Error: Failed to clone talawa-api repository from branch '$FULL_BRANCH_NAME'" echo "This could be due to network issues or repository permissions" + rm -rf talawa-api || true exit 1 fi # Verify clone was successful if [ ! -d "talawa-api" ]; then echo "Error: talawa-api directory not found after clone" exit 1 fi🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized modification or deletion of sensitive file. This file requires special permissions. To proceed, apply the 'ignore-sensitive-files-pr' label.
332-346
: Enhance GraphQL document validation.Consider improving the validation step with:
- More descriptive error messages
- Support for both .graphql files and embedded GraphQL in TypeScript
- name: Validate Documents run: | if [ ! -f './talawa-api/schema.graphql' ]; then echo "Error: schema.graphql not found in talawa-api" exit 1 fi GRAPHQL_FILES=$(find ./src/GraphQl -type f \( -name "*.ts" -o -name "*.tsx" \)) if [ -z "$GRAPHQL_FILES" ]; then echo "Error: No GraphQL TypeScript files found" exit 1 fi echo "Found GraphQL files:" echo "$GRAPHQL_FILES" - graphql-inspector validate './src/GraphQl/**/*.{ts,tsx}' './talawa-api/schema.graphql' + # Check both .graphql files and TypeScript files + if ! graphql-inspector validate './src/GraphQl/**/*.{ts,tsx,graphql}' './talawa-api/schema.graphql'; then + echo "Error: GraphQL validation failed. Please check:" + echo "1. All queries and mutations match the schema" + echo "2. All required fields are included" + echo "3. No deprecated fields are used" + exit 1 + fi🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized modification or deletion of sensitive file. This file requires special permissions. To proceed, apply the 'ignore-sensitive-files-pr' label.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml
(1 hunks)
🧰 Additional context used
📓 Learnings (1)
.github/workflows/pull-request.yml (1)
Learnt from: Priyanshuthapliyal2005
PR: PalisadoesFoundation/talawa-admin#3496
File: .github/workflows/pull-request.yml:0-0
Timestamp: 2025-01-31T14:05:10.575Z
Learning: The talawa-admin repository standardizes on Node.js v22.x across all workflows and documentation.
🪛 YAMLlint (1.35.1)
.github/workflows/pull-request.yml
[error] 275-275: trailing spaces
(trailing-spaces)
[error] 307-307: trailing spaces
(trailing-spaces)
🪛 GitHub Actions: PR Workflow
.github/workflows/pull-request.yml
[error] Unauthorized modification or deletion of sensitive file. This file requires special permissions. To proceed, apply the 'ignore-sensitive-files-pr' label.
🔇 Additional comments (1)
.github/workflows/pull-request.yml (1)
292-303
: LGTM! Job configuration follows repository standards.The job is properly configured with:
- Correct Node.js version (v22.x)
- Appropriate condition to skip for dependabot
🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized modification or deletion of sensitive file. This file requires special permissions. To proceed, apply the 'ignore-sensitive-files-pr' label.
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.
See comments
b4810e2
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)
.github/workflows/pull-request.yml (2)
275-275
: Remove Trailing Spaces
A trailing space is detected on line 275. Please remove it to adhere to YAML formatting standards and avoid linting errors.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 275-275: trailing spaces
(trailing-spaces)
292-292
: Remove Trailing Spaces
Trailing spaces are detected on line 292. Please remove them to clean up the YAML file and prevent linting issues.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 292-292: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml
(1 hunks)
🧰 Additional context used
📓 Learnings (1)
.github/workflows/pull-request.yml (1)
Learnt from: Priyanshuthapliyal2005
PR: PalisadoesFoundation/talawa-admin#3496
File: .github/workflows/pull-request.yml:0-0
Timestamp: 2025-01-31T14:05:10.575Z
Learning: The talawa-admin repository standardizes on Node.js v22.x across all workflows and documentation.
🪛 YAMLlint (1.35.1)
.github/workflows/pull-request.yml
[error] 275-275: trailing spaces
(trailing-spaces)
[error] 292-292: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test Application
- GitHub Check: Check Python Code Style
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (6)
.github/workflows/pull-request.yml (6)
276-280
: Graphql-Inspector Job Header
The new Graphql-Inspector job is properly configured with a dependency on the Test-Application job and includes a condition to skip execution for dependabot PRs. This setup aligns well with the introspection testing objectives.
281-284
: Repository Checkout Step
This step checks out the repository using the standard action, ensuring that the latest code is available for the GraphQL inspection process.
285-288
: Node.js Setup for GraphQL Inspector
The Node.js setup uses version "22.x", which is consistent with the repository’s standard. This configuration is appropriate for this job.
290-291
: Dependency Installation for GraphQL Inspector
The step installs the latest @graphql-inspector/cli globally, ensuring that the necessary tool for schema validation is available. Consider renaming the step to "Install GraphQL Inspector" for improved clarity.
293-316
: Clone API Repository Script
This script retrieves the branch name from the PR’s base reference, verifies that the specified branch exists in the talawa-api repository, and then clones it with proper error handling. The error messages are clear and useful. Adding a confirmation log after a successful clone might further enhance traceability.
317-331
: GraphQL Documents Validation
This step verifies that the './talawa-api/schema.graphql' file exists and checks for the presence of GraphQL TypeScript files within './src/GraphQl'. The invocation of graphql-inspector for schema validation is correctly implemented.
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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 (4)
.github/workflows/pull-request.yml (4)
275-275
: Remove Trailing Whitespace
A trailing whitespace was detected on this line. Please remove it to comply with YAML linting rules.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 275-275: trailing spaces
(trailing-spaces)
290-291
: Global Dependency Installation
Installing@graphql-inspector/cli
globally is acceptable. Consider caching this installation in the future to speed up builds if this step becomes a performance bottleneck.
292-292
: Remove Trailing Whitespace
A trailing whitespace was detected on this line as well. Please remove it to align with YAML linting requirements.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 292-292: trailing spaces
(trailing-spaces)
293-316
: Clone API Repository Step – Quote Variables
The cloning step is well structured with proper error handling. However, for better robustness, it is recommended to quote the branch variable ($FULL_BRANCH_NAME
) in the git commands to prevent potential issues with special characters or spaces in branch names.Here’s a diff suggestion:
-if ! git ls-remote --heads https://github.com/PalisadoesFoundation/talawa-api.git $FULL_BRANCH_NAME | grep -q .; then +if ! git ls-remote --heads https://github.com/PalisadoesFoundation/talawa-api.git "$FULL_BRANCH_NAME" | grep -q .; then-if ! git clone --branch $FULL_BRANCH_NAME --depth 1 https://github.com/PalisadoesFoundation/talawa-api; then +if ! git clone --branch "$FULL_BRANCH_NAME" --depth 1 https://github.com/PalisadoesFoundation/talawa-api; then
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml
(1 hunks)
🧰 Additional context used
📓 Learnings (1)
.github/workflows/pull-request.yml (1)
Learnt from: Priyanshuthapliyal2005
PR: PalisadoesFoundation/talawa-admin#3496
File: .github/workflows/pull-request.yml:0-0
Timestamp: 2025-01-31T14:05:10.575Z
Learning: The talawa-admin repository standardizes on Node.js v22.x across all workflows and documentation.
🪛 YAMLlint (1.35.1)
.github/workflows/pull-request.yml
[error] 275-275: trailing spaces
(trailing-spaces)
[error] 292-292: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (3)
.github/workflows/pull-request.yml (3)
276-279
: Graphql-Inspector Job Definition
The new job is correctly defined with a dependency on theTest-Application
job and an appropriate conditional to skip dependabot PRs. Ensure that the job name and description accurately reflect its purpose of running GraphQL introspection on the specified repository.
285-288
: Node.js Version Setup
The job sets up Node.js usingactions/setup-node@v4
with version22.x
, which is consistent with the repository’s standard. This is correct given the current project guidelines.
317-331
: Validate Documents Step – Glob Patterns and File Checks
The validation step appropriately checks for the existence ofschema.graphql
in the cloned repository and ensures that GraphQL TypeScript files exist before runninggraphql-inspector
. The glob pattern ('./src/GraphQl/**/*.{ts,tsx}'
) appears well formulated; however, verify that it matches all intended files according to your project’s directory structure.
I have some questions.
|
Yeah that’s a great idea! We can use a similar schema.graphql here. Due to the changes in the develop-postgres branch, the repo's coverage on Codecov is currently low. Mainly, the screens that rely heavily on MongoDB directly will not work, while others should function as expected. |
|
Should I create a new issue and PR for this, or continue making the changes here? |
This one |
ccc9dd6
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 (4)
.github/workflows/pull-request.yml (4)
290-292
: Dependency Installation: Consider Pinning VersionThe step installs the latest version of the
@graphql-inspector/cli
globally. While using@latest
ensures you get the newest features, it might introduce unpredictable changes. For better reproducibility, consider pinning to a known stable version. For example:- - name: resolve dependency - run: npm install -g @graphql-inspector/cli@latest + - name: resolve dependency + run: npm install -g @graphql-inspector/[email protected] # Pin to a stable version🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 292-292: trailing spaces
(trailing-spaces)
293-317
: Commented Cloning Block: Remove or DocumentThere is a large commented-out section that previously handled cloning the API repository. Since the job now operates entirely on a local
schema.graphql
file, consider either removing this block to reduce clutter or adding a clarifying comment (e.g., a TODO note) explaining that it is retained for historical context.
318-332
: Validate Documents Step: Update Error Message for ClarityThe validation step correctly checks for the existence of
./schema.graphql
and for GraphQL-related TypeScript files. However, the error message:echo "Error: schema.graphql not found in talawa-api"
could be misleading now that the job uses a local file. It may be clearer to change it to something like:
- if [ ! -f './schema.graphql' ]; then - echo "Error: schema.graphql not found in talawa-api" + if [ ! -f './schema.graphql' ]; then + echo "Error: schema.graphql not found in the repository root"This small change would improve clarity for future maintainers.
275-275
: Remove Trailing SpacesStatic analysis has flagged trailing whitespace on line 275 and again on line 292. Please remove these extraneous spaces to maintain clean YAML formatting.
Also applies to: 292-292
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 275-275: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml
(1 hunks)
🧰 Additional context used
📓 Learnings (1)
.github/workflows/pull-request.yml (1)
Learnt from: Priyanshuthapliyal2005
PR: PalisadoesFoundation/talawa-admin#3496
File: .github/workflows/pull-request.yml:0-0
Timestamp: 2025-01-31T14:05:10.575Z
Learning: The talawa-admin repository standardizes on Node.js v22.x across all workflows and documentation.
🪛 YAMLlint (1.35.1)
.github/workflows/pull-request.yml
[error] 275-275: trailing spaces
(trailing-spaces)
[error] 292-292: trailing spaces
(trailing-spaces)
🔇 Additional comments (2)
.github/workflows/pull-request.yml (2)
276-280
: Graphql-Inspector Job Header Defined CorrectlyThe new job header correctly establishes the Graphql-Inspector job with a dependency on the Test-Application job and runs conditionally (excluding dependabot). This aligns with the PR objective to restore the GraphQL introspection test.
285-289
: Node.js Setup Step Matches Repository StandardsThe Node.js setup uses version "22.x", which is consistent with the repository’s standard (as confirmed by long-term learnings). No changes needed here.
1ff6798
I have replaced the current schema.graphql with the one from the develop-postgres branch of talawa-api, commented out the clone step for talawa-api since it's no longer needed when using the schema directly, and updated the schema path accordingly. |
1ff6798
to
55e6630
Compare
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)
.github/workflows/pull-request.yml (3)
275-332
: New Graphql-Inspector Job – Overall Structure and Validation LogicThe new
Graphql-Inspector
job is well structured:
• It correctly depends on theTest-Application
job and uses the standardized Node.js version (22.x
).
• The dependency on@graphql-inspector/cli
is resolved via a global install, ensuring the tool is available.
• The "Validate Documents" step properly checks for the presence of a localschema.graphql
file and confirms that GraphQL TypeScript files exist in./src/GraphQl
before running validation.Suggestion:
Consider rewording the error message in the validation step to avoid referencing "talawa-api"—since the local file is now used. For example, change:-if [ ! -f './schema.graphql' ]; then - echo "Error: schema.graphql not found in talawa-api" - exit 1 +if [ ! -f './schema.graphql' ]; then + echo "Error: schema.graphql not found in the repository" + exit 1🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 275-275: trailing spaces
(trailing-spaces)
[error] 292-292: trailing spaces
(trailing-spaces)
293-317
: Cleanup of Legacy Cloned Repository CodeThere is a block of commented-out code (lines 293–317) that was previously used for cloning the API repository. Since the job now uses a local
schema.graphql
file, removing this commented-out section could reduce clutter and improve long-term maintainability.
275-275
: Remove Trailing WhitespaceStatic analysis has flagged trailing spaces on lines 275 and 292. Please remove these extra spaces to conform to YAML linting guidelines. For example:
- node-version: '22.x'·· + node-version: '22.x'Also applies to: 292-292
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 275-275: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml
(1 hunks)
🧰 Additional context used
📓 Learnings (1)
.github/workflows/pull-request.yml (1)
Learnt from: Priyanshuthapliyal2005
PR: PalisadoesFoundation/talawa-admin#3496
File: .github/workflows/pull-request.yml:0-0
Timestamp: 2025-01-31T14:05:10.575Z
Learning: The talawa-admin repository standardizes on Node.js v22.x across all workflows and documentation.
🪛 YAMLlint (1.35.1)
.github/workflows/pull-request.yml
[error] 275-275: trailing spaces
(trailing-spaces)
[error] 292-292: trailing spaces
(trailing-spaces)
3e31b31
to
55e6630
Compare
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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)
.github/workflows/pull-request.yml (3)
292-316
: Remove Unused Commented-out Clone Code
The block of commented-out code (lines 292–316) that handles cloning from the API repository is now obsolete since the workflow uses a localschema.graphql
file. Removing this commented-out block will reduce clutter and improve maintainability.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 292-292: trailing spaces
(trailing-spaces)
277-278
: Clarify Job Description
The job’s name/description currently states:"Runs Introspection on the GitHub talawa-api repo on the schema.graphql file"
Since the current implementation relies on a localschema.graphql
file rather than performing a repository clone, updating this description to reflect that would prevent potential confusion.
317-331
: Clarify Local Schema Error Message in Validate Documents Step
Within the Validate Documents step, the error message reads:"Error: schema.graphql not found in talawa-api"
Given that the workflow now expects theschema.graphql
file to be located in the repository root, this message could be misleading. It is recommended to update the message to something like:
"Error: schema.graphql not found in the repository root"
This change will provide clearer feedback to the user in case the file is missing.Suggested diff:
- if [ ! -f './schema.graphql' ]; then - echo "Error: schema.graphql not found in talawa-api" + if [ ! -f './schema.graphql' ]; then + echo "Error: schema.graphql not found in the repository root"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml
(1 hunks)
🧰 Additional context used
📓 Learnings (1)
.github/workflows/pull-request.yml (1)
Learnt from: Priyanshuthapliyal2005
PR: PalisadoesFoundation/talawa-admin#3496
File: .github/workflows/pull-request.yml:0-0
Timestamp: 2025-01-31T14:05:10.575Z
Learning: The talawa-admin repository standardizes on Node.js v22.x across all workflows and documentation.
🪛 YAMLlint (1.35.1)
.github/workflows/pull-request.yml
[error] 275-275: trailing spaces
(trailing-spaces)
[error] 292-292: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (1)
.github/workflows/pull-request.yml (1)
275-292
: New Graphql-Inspector Job Setup and Trailing Spaces Fix
The newly added Graphql-Inspector job correctly sets up the environment to perform GraphQL introspection using the localschema.graphql
file. The dependency installation withnpm install -g @graphql-inspector/cli@latest
is properly included, and the job is made to run only after theTest-Application
job and when the actor is notdependabot[bot]
.However, static analysis detected trailing spaces on these lines (notably on lines 275 and 292). Please remove these extra spaces to ensure YAML formatting consistency. Additionally, consider whether the commented-out cloning section (below) is still needed.
Suggested diff for trailing spaces cleanup:
- Graphql-Inspector:␣ + Graphql-Inspector:🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 275-275: trailing spaces
(trailing-spaces)
[error] 292-292: trailing spaces
(trailing-spaces)
What kind of change does this PR introduce?
NoMongo: Restore the GraphQL Introspection GitHub Action Test
Issue Number:
Fixes #3235
Snapshots/Videos:
If relevant, did you update the documentation?
Summary
Does this PR introduce a breaking change?
Checklist
CodeRabbit AI Review
Test Coverage
Other information
Have you read the contributing guide?
Summary by CodeRabbit
Graphql-Inspector
, to validate the GraphQL schema in the pull request workflow.src/GraphQl
directory.