Skip to content

ci: upstream change#4263

Closed
ruthra-kumar wants to merge 1 commit intofrappe:developfrom
ruthra-kumar:test_upstream_change
Closed

ci: upstream change#4263
ruthra-kumar wants to merge 1 commit intofrappe:developfrom
ruthra-kumar:test_upstream_change

Conversation

@ruthra-kumar
Copy link
Copy Markdown
Member

@ruthra-kumar ruthra-kumar commented Mar 24, 2026

Summary by CodeRabbit

  • Chores
    • Updated the ERPNext app source in the CI/CD installation process to use a specific branch variant for testing purposes.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 24, 2026

Walkthrough

The install script's ERPNext application retrieval process was modified. The cloning source was changed from a dynamic repository path using the frappeuser variable to a fixed repository URL pointing to a specific fork. Additionally, the target branch was changed from a variable reference to a fixed branch name. The dependency resolution flag remains in place, and all other installation steps are unaffected by these modifications.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'ci: upstream change' is vague and generic, using non-descriptive language that does not clearly convey what upstream change was made to the CI configuration. Use a more specific title that describes the actual change, such as 'ci: update ERPNext clone source to ruthra-kumar fork' or 'ci: pin ERPNext branch for testing'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a 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

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/helper/install.sh:
- Around line 51-52: The bench get-app invocation was changed to a hardcoded
fork/branch; revert it to use the parameterized variables so CI/env vars work.
Replace the hardcoded bench get-app line with the parameterized form that uses
$frappeuser (or FRAPPE_USER) and $erpnextbranch (or ERPNEXT_BRANCH) so ERPNext
is installed consistently like the other apps; update the bench get-app call
(the one currently calling "ruthra-kumar/erpnext" and branch
"move_commits_inside_test_guard_clause") to the original parameterized
invocation that references $frappeuser and $erpnextbranch.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1e551e95-4582-4c49-8948-6b178caa92c3

📥 Commits

Reviewing files that changed from the base of the PR and between 77beb3b and b789e02.

📒 Files selected for processing (1)
  • .github/helper/install.sh

Comment on lines +51 to +52
# bench get-app "https://github.com/${frappeuser}/erpnext" --branch "$erpnextbranch" --resolve-deps
bench get-app "https://github.com/ruthra-kumar/erpnext" --branch "move_commits_inside_test_guard_clause" --resolve-deps
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Debug code should not be merged: Hardcoded personal fork and test branch.

This change replaces the parameterized ERPNext source with a hardcoded personal fork (ruthra-kumar/erpnext) and a test-specific branch (move_commits_inside_test_guard_clause). This appears to be temporary test code that should not be committed to the develop branch:

  1. Bypasses CI parameterization: The $frappeuser and $erpnextbranch variables (lines 14, 16) are now ignored for ERPNext, breaking the FRAPPE_USER/ERPNEXT_BRANCH environment variable support used by ci.yml and run-individual-tests.yml.

  2. Creates inconsistency: Frappe (line 20), payments (line 50), and lending (line 53) still use parameterized URLs, while ERPNext now uses a hardcoded fork.

  3. Branch name indicates test code: The branch name move_commits_inside_test_guard_clause suggests this is for testing specific changes, not a permanent CI configuration.

If you need to test against a fork temporarily, consider using the existing FRAPPE_USER mechanism or reverting before merge.

Suggested fix: Revert to parameterized command
-# bench get-app "https://github.com/${frappeuser}/erpnext" --branch "$erpnextbranch" --resolve-deps
-bench get-app "https://github.com/ruthra-kumar/erpnext" --branch "move_commits_inside_test_guard_clause" --resolve-deps
+bench get-app "https://github.com/${frappeuser}/erpnext" --branch "$erpnextbranch" --resolve-deps
📝 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.

Suggested change
# bench get-app "https://github.com/${frappeuser}/erpnext" --branch "$erpnextbranch" --resolve-deps
bench get-app "https://github.com/ruthra-kumar/erpnext" --branch "move_commits_inside_test_guard_clause" --resolve-deps
bench get-app "https://github.com/${frappeuser}/erpnext" --branch "$erpnextbranch" --resolve-deps
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/helper/install.sh around lines 51 - 52, The bench get-app invocation
was changed to a hardcoded fork/branch; revert it to use the parameterized
variables so CI/env vars work. Replace the hardcoded bench get-app line with the
parameterized form that uses $frappeuser (or FRAPPE_USER) and $erpnextbranch (or
ERPNEXT_BRANCH) so ERPNext is installed consistently like the other apps; update
the bench get-app call (the one currently calling "ruthra-kumar/erpnext" and
branch "move_commits_inside_test_guard_clause") to the original parameterized
invocation that references $frappeuser and $erpnextbranch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant