Docs: clarify upstream remote setup and fork sync steps for contributors#3234
Docs: clarify upstream remote setup and fork sync steps for contributors#3234SuyashJain17 wants to merge 1 commit intoOWASP:mainfrom
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis pull request updates Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/run-ci-cd.yaml (2)
153-185: Critical: Missingactions: writepermission for GHA cache writes.The
run-backend-testsjob usescache-to: type=gha(line 175) but only hascontents: readpermission. With the new top-levelpermissions: {}, this job will fail to write to the GitHub Actions cache.🔒 Add actions: write permission
run-backend-tests: name: Run backend tests needs: - scan-code - scan-ci-dependencies permissions: + actions: write contents: read runs-on: ubuntu-latest
187-219: Critical: Missingactions: writepermission for GHA cache writes.The
run-frontend-unit-testsjob usescache-to: type=gha(line 209) but only hascontents: readpermission. With the new top-levelpermissions: {}, this job will fail to write to the GitHub Actions cache.🔒 Add actions: write permission
run-frontend-unit-tests: name: Run frontend unit tests needs: - scan-code - scan-ci-dependencies permissions: + actions: write contents: read runs-on: ubuntu-latestCONTRIBUTING.md (1)
533-582: Resolve section numbering inconsistency.The heading structure is inconsistent: "Find Something to Work On" and "Create a Branch" are unnumbered (lines 533, 540), but subsequent sections jump to "### 3. Make Changes and Commit" (line 548). Removing the "2." prefix from "Create a Branch" without renumbering the downstream sections creates a logical gap.
Choose one of the following:
- Number all sections consistently (1–6): Rename to "1. Find Something...", "2. Create a Branch", "3. Make Changes...", etc.
- Nest subsections under "Create a Branch": Change lines 548, 565, 573, 578 to
####(4th level) so they appear as sub-steps of "Create a Branch" rather than top-level sections.- Remove all numbering: Strip the leading numbers from lines 548, 565, 573, 578 for a flat, unnumbered structure.
🤖 Fix all issues with AI agents
In @CONTRIBUTING.md:
- Line 535: Update the GitHub URL text on the referenced line to use the same
capitalization as other references: replace
"https://github.com/owasp/nest/issues" with
"https://github.com/OWASP/Nest/issues" so the repository owner/name is
capitalized as OWASP/Nest for consistency across CONTRIBUTING.md.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/check-pr-issue.yaml.github/workflows/label-issues.yaml.github/workflows/label-pull-requests.yaml.github/workflows/run-ci-cd.yaml.github/workflows/run-code-ql.yaml.github/workflows/update-nest-test-images.yamlCONTRIBUTING.md
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-21T19:03:59.068Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1139
File: .github/workflows/setup-backend-environment/action.yaml:16-27
Timestamp: 2025-12-21T19:03:59.068Z
Learning: Composite actions (runs: using: composite) execute as steps within the calling job's context and can access the job context, including job.services.* properties (e.g., job.services.<service_id>.id, job.services.<service_id>.ports). Service containers must be defined at the job level, but a composite action's steps can reference them via the job context.
Applied to files:
.github/workflows/update-nest-test-images.yaml.github/workflows/run-ci-cd.yaml.github/workflows/label-pull-requests.yaml.github/workflows/check-pr-issue.yaml.github/workflows/label-issues.yaml.github/workflows/run-code-ql.yaml
📚 Learning: 2026-01-05T16:20:39.976Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 3205
File: docker-compose/local.yaml:32-32
Timestamp: 2026-01-05T16:20:39.976Z
Learning: In the OWASP/Nest repository, feature branches use unique volume name suffixes in docker-compose files to prevent volume clashes across parallel development efforts. For example, the feature/nest-zappa-migration branch uses `-zappa` suffix for all volume names (backend-venv-zappa, cache-data-zappa, db-data-zappa, etc.) to ensure isolated environments when switching between branches.
Applied to files:
CONTRIBUTING.md
📚 Learning: 2025-07-28T14:51:14.736Z
Learnt from: adithya-naik
Repo: OWASP/Nest PR: 1894
File: frontend/src/components/TopContributorsList.tsx:74-74
Timestamp: 2025-07-28T14:51:14.736Z
Learning: In the OWASP/Nest project, the maintainer adithya-naik prefers not to create separate components for code that's only used in two specific cases, following the YAGNI principle to avoid over-engineering when the duplication is limited and manageable.
Applied to files:
CONTRIBUTING.md
🔇 Additional comments (9)
.github/workflows/update-nest-test-images.yaml (2)
8-8: LGTM: Security best practice applied.Setting top-level
permissions: {}follows the principle of least privilege, requiring each job to explicitly declare needed permissions.
19-19: LGTM: Required permission for GHA cache writes.The
actions: writepermission is necessary for the job to write to GitHub Actions cache (cache-to: type=ghaat lines 40, 55, 70)..github/workflows/label-pull-requests.yaml (1)
6-6: LGTM: Security best practice applied.Setting top-level
permissions: {}follows the principle of least privilege. The existing job-level permissions remain functional..github/workflows/run-code-ql.yaml (1)
15-15: LGTM: Security best practice applied.Setting top-level
permissions: {}follows the principle of least privilege. The existing job-level permissions remain functional..github/workflows/run-ci-cd.yaml (2)
22-22: LGTM: Security best practice applied.Setting top-level
permissions: {}follows the principle of least privilege.However, verify that jobs using
cache-to: type=ghahave the necessaryactions: writepermission. See the next comment for details.
269-386: Critical: Missingactions: writepermission for GHA cache writes.The
build-staging-imagesjob usescache-to: type=registry(lines 311, 359) which doesn't requireactions: write, but it also usescache-to: type=ghaimplicitly. However, reviewing more carefully, lines 310 and 359 showtype=registryfor cache-to, nottype=gha.Wait, let me re-examine. Lines 308-311 show:
cache-to: | type=registry,ref=owasp/nest:backend-staging-cacheAnd lines 357-359 show:
cache-to: | type=registry,ref=owasp/nest:frontend-staging-cacheThese use registry-type cache, not GHA cache, so they don't need
actions: write. The previous comments about run-backend-tests and run-frontend-unit-tests remain valid..github/workflows/check-pr-issue.yaml (1)
8-8: LGTM: Security best practice applied.Setting top-level
permissions: {}follows the principle of least privilege. The existing job-level permissions remain functional..github/workflows/label-issues.yaml (1)
1-44: LGTM on permissions structure and YAML refactor.The top-level
permissions: {}combined with explicit job-levelissues: writeis the correct approach for least-privilege access scoping in GitHub Actions. The YAML restructuring is sound and maintains functional equivalence with the prior version.CONTRIBUTING.md (1)
495-531: Excellent addition to clarify upstream sync workflow.The new "Keep Your Fork in Sync with Upstream" section clearly explains the one-time setup and recurring sync steps with appropriate git commands and rationale. This directly addresses the PR objective to reduce issues from outdated forks and unnecessary rebases during review.
SuyashJain17
force-pushed
the
docs-upstream-fork-sync
branch
from
163454e to
faef0eb
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @CONTRIBUTING.md:
- Line 535: The repository URL text 'https://github.com/owasp/nest/issues' is
lowercased and should be updated to use the same capitalization as other
references; replace that string with 'https://github.com/OWASP/Nest/issues' so
the repository reference reads OWASP/Nest consistently in the CONTRIBUTING.md
(the specific occurrence is the URL text on the checked line).
🧹 Nitpick comments (1)
CONTRIBUTING.md (1)
533-581: Inconsistent workflow step numbering creates ambiguity.The "Keep Your Fork in Sync" section uses internal numbering (1, 2), but then "Find Something to Work On" and "Create a Branch" are unnumbered, while subsequent sections jump to numbers 3–6. This layout is unclear about whether all five items (sync, find, branch, commit, push, PR, merge) form a sequential workflow or if the numbering applies only to the later steps.
Consider either:
- Renumber all main workflow steps consistently (1–7), treating "Keep Your Fork in Sync" as step 1, or
- Remove numbers from all workflow steps for clarity and consistency.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CONTRIBUTING.md
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-06T12:48:44.916Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2223
File: backend/apps/owasp/models/common.py:0-0
Timestamp: 2025-09-06T12:48:44.916Z
Learning: When migrating scraper logic to GitHub .md file parsing in the OWASP project, the new implementation should maintain the same case-sensitivity behavior as the original scraper logic, even if case-insensitive matching might seem more robust.
Applied to files:
CONTRIBUTING.md
📚 Learning: 2025-07-28T14:51:14.736Z
Learnt from: adithya-naik
Repo: OWASP/Nest PR: 1894
File: frontend/src/components/TopContributorsList.tsx:74-74
Timestamp: 2025-07-28T14:51:14.736Z
Learning: In the OWASP/Nest project, the maintainer adithya-naik prefers not to create separate components for code that's only used in two specific cases, following the YAGNI principle to avoid over-engineering when the duplication is limited and manageable.
Applied to files:
CONTRIBUTING.md
📚 Learning: 2025-06-18T20:00:23.899Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1634
File: frontend/src/app/api/auth/[...nextauth]/route.ts:30-55
Timestamp: 2025-06-18T20:00:23.899Z
Learning: The OWASP Nest application has logging disabled, so avoid suggesting console.log, console.error, or any other logging statements in code review suggestions.
Applied to files:
CONTRIBUTING.md
📚 Learning: 2026-01-05T16:20:39.976Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 3205
File: docker-compose/local.yaml:32-32
Timestamp: 2026-01-05T16:20:39.976Z
Learning: In the OWASP/Nest repository, feature branches use unique volume name suffixes in docker-compose files to prevent volume clashes across parallel development efforts. For example, the feature/nest-zappa-migration branch uses `-zappa` suffix for all volume names (backend-venv-zappa, cache-data-zappa, db-data-zappa, etc.) to ensure isolated environments when switching between branches.
Applied to files:
CONTRIBUTING.md
🔇 Additional comments (1)
CONTRIBUTING.md (1)
495-531: ✅ Excellent addition of fork-sync documentation.The new "Keep Your Fork in Sync with Upstream" section clearly explains the problem (outdated forks) and provides straightforward one-time setup and pre-work sync instructions with correct git commands. This directly addresses the issue objective and should help contributors avoid merge conflicts and rebase requests.
|
|
||
| ### Find Something to Work On | ||
|
|
||
| - Check the **Issues** tab for open issues: [https://github.com/owasp/nest/issues](https://github.com/owasp/nest/issues) |
There was a problem hiding this comment.
Capitalize repository reference for consistency.
Line 535 uses lowercase owasp/nest in the GitHub URL, while all other OWASP/Nest references in this file (lines 30, 34, 379, 409, 410, 510) use proper capitalization OWASP/Nest. Update for consistency:
- Check the **Issues** tab for open issues: [https://github.com/owasp/nest/issues](https://github.com/owasp/nest/issues)
+ Check the **Issues** tab for open issues: [https://github.com/OWASP/Nest/issues](https://github.com/OWASP/Nest/issues)Also applies to: 535-535
🤖 Prompt for AI Agents
In @CONTRIBUTING.md at line 535, The repository URL text
'https://github.com/owasp/nest/issues' is lowercased and should be updated to
use the same capitalization as other references; replace that string with
'https://github.com/OWASP/Nest/issues' so the repository reference reads
OWASP/Nest consistently in the CONTRIBUTING.md (the specific occurrence is the
URL text on the checked line).
|
Closing this PR in favor of a clean replacement with a focused diff. I’ve opened a new PR from a fresh branch that contains only the intended documentation changes and avoids unrelated commits. Thanks! |
1 participant
Proposed change
Resolves #3221
This PR adds a small, documentation-only section to
CONTRIBUTING.mdclarifying how contributors can:upstreamremote for the main OWASP/Nest repositorymainbranch withupstream/mainbefore starting new workThe intent is to document the expected fork-sync workflow in one place, helping contributors avoid working from outdated forks and reducing rebase requests during review.
Checklist
make check-testlocally and all tests passed