-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Codecov work #2888
Codecov work #2888
Conversation
WalkthroughThe pull request modifies the GitHub Actions workflow file for the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (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
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: 1
🔭 Outside diff range comments (2)
.github/workflows/push.yml (2)
Line range hint
23-34
: Consolidate coverage upload steps to avoid redundancy.The workflow appears to upload coverage data twice:
- During the test run (implicitly through environment variable)
- Using the codecov action
This redundancy might cause issues with coverage reporting.
Consolidate the coverage upload by removing the
CODECOV_TOKEN
from the test step and relying solely on the codecov action:- name: Run Tests and Upload Coverage to Codecov run: | pnpm install --frozen-lockfile pnpm run test - env: - CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}🧰 Tools
🪛 actionlint (1.7.4)
15-15: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
Line range hint
31-34
: Consider adding fail_ci_if_error flag.To ensure CI fails if coverage upload fails, which helps catch coverage reporting issues early.
Add the flag to the codecov action:
- name: Upload Coverage to Codecov uses: codecov/codecov-action@v4 with: token: ${{ secrets.CODECOV_TOKEN }} verbose: true files: ./coverage/lcov.info + fail_ci_if_error: true
🧰 Tools
🪛 actionlint (1.7.4)
15-15: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/push.yml
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2888 +/- ##
===================================================
Coverage ? 39.85%
===================================================
Files ? 453
Lines ? 33203
Branches ? 385
===================================================
Hits ? 13234
Misses ? 19969
Partials ? 0 ☔ View full report in Codecov by Sentry. |
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.
Use npm to install pnpm
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.
- I discovered pnpm has its own github action for setup. Please use that instead
- Look at pull-request.yml
pnpm/action-setup@v4
.github/workflows/push.yml
Outdated
@@ -14,6 +14,10 @@ jobs: | |||
- name: Checkout code | |||
uses: actions/checkout@v3 | |||
|
|||
# Install pnpm | |||
- name: Install pnpm |
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.
- I discovered pnpm has its own github action for setup. Please use that instead
- Look at pull-request.yml
pnpm/action-setup@v4
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: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/push.yml
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (1)
.github/workflows/push.yml (1)
Line range hint
27-41
: LGTM! Codecov integration is properly configured.The Codecov setup follows best practices:
- Uses environment variables for secure token handling
- Enables verbose output for better debugging
- Specifies the coverage file path explicitly
🧰 Tools
🪛 actionlint (1.7.4)
15-15: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/push.yml
Outdated
# Setup pnpm | ||
- name: Setup pnpm | ||
uses: pnpm/action-setup@v4 | ||
with: | ||
cache: pnpm | ||
cache-dependency-path: | | ||
docs/pnpm-lock.yaml | ||
docs/package.json | ||
|
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.
🧹 Nitpick (assertive)
Consider adding pnpm version constraint.
While the setup uses the official pnpm action as recommended, it's good practice to specify a version to ensure consistent builds.
- name: Setup pnpm
uses: pnpm/action-setup@v4
with:
+ version: 8
cache: pnpm
cache-dependency-path: |
docs/pnpm-lock.yaml
docs/package.json
📝 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.
# Setup pnpm | |
- name: Setup pnpm | |
uses: pnpm/action-setup@v4 | |
with: | |
cache: pnpm | |
cache-dependency-path: | | |
docs/pnpm-lock.yaml | |
docs/package.json | |
# Setup pnpm | |
- name: Setup pnpm | |
uses: pnpm/action-setup@v4 | |
with: | |
version: 8 | |
cache: pnpm | |
cache-dependency-path: | | |
docs/pnpm-lock.yaml | |
docs/package.json |
.github/workflows/push.yml
Outdated
# Setup pnpm | ||
- name: Setup pnpm | ||
uses: pnpm/action-setup@v4 | ||
with: | ||
cache: pnpm | ||
cache-dependency-path: | | ||
docs/pnpm-lock.yaml | ||
docs/package.json |
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.
💡 Codebase verification
Cache configuration is incorrect and needs to include root dependencies
The current cache only includes docs directory dependencies, but the workflow runs pnpm install
and tests in the root directory. Update the cache configuration to:
cache-dependency-path: |
pnpm-lock.yaml
package.json
🔗 Analysis chain
Verify the cache dependency paths.
The cache configuration appears to be looking for pnpm files only in the docs
directory. This might not be correct if your project's main dependencies are in the root directory.
Let's verify the correct location of the pnpm files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for pnpm files in both root and docs directory
echo "Checking pnpm files location..."
fd -t f "pnpm-lock.yaml|package.json" -d 2
Length of output: 183
Script:
#!/bin/bash
# Examine the complete workflow file
cat .github/workflows/push.yml
Length of output: 1030
.github/workflows/push.yml
Outdated
uses: pnpm/action-setup@v4 | ||
with: | ||
cache: pnpm | ||
cache-dependency-path: | |
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.
Remove. This was specific to deploying the website
.github/workflows/push.yml
Outdated
with: | ||
cache: pnpm | ||
cache-dependency-path: | | ||
docs/pnpm-lock.yaml |
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.
Remove. This was specific to deploying the website
.github/workflows/push.yml
Outdated
cache: pnpm | ||
cache-dependency-path: | | ||
docs/pnpm-lock.yaml | ||
docs/package.json |
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.
Remove. This was specific to deploying the website
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
|
@palisadoes are you asking me to remove this |
ed57c24
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
bugfix
Issue Number:
Fixes #2854
Previous Merge Related to this issue #2877
Snapshots/Videos:
If relevant, did you update the documentation?
Summary
I have updated the
push-deploy-website.yml
file to send Codecov data to the Codecov website after a merge.Does this PR introduce a breaking change?
Checklist
CodeRabbit AI Review
Test Coverage
Other information
Have you read the contributing guide?
Yes
Summary by CodeRabbit
Summary by CodeRabbit
pnpm
in the GitHub Actions workflow for automated testing and coverage reporting on thedevelop-postgres
branch.