chore: cherry pick git executor revert#39954
Conversation
## Description > > _Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR._ Fixes #39934 _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.Git" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/14103475195> > Commit: 288e4cf > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=14103475195&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git` > Spec: > <hr>Thu, 27 Mar 2025 10:37:32 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Enhanced error recovery in update operations, ensuring that any pending changes are safely reverted when issues occur. - Improved conflict resolution during updates to maintain a stable and reliable system state for end-users. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
WalkthroughThe changes enhance error handling in the Git operations within the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GitExecutor as GitExecutorCEImpl
participant Git as Git Repository
Client->>GitExecutor: pullApplication()
GitExecutor->>Git: Initiate pull operation
alt Error Occurs
GitExecutor->>Git: resetToLastCommit()
GitExecutor->>Client: Return error after reset
else No Error
GitExecutor->>Client: Return success
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 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
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/server/appsmith-git/src/main/java/com/appsmith/git/service/ce/GitExecutorCEImpl.java (2)
502-502: Consider logging rollback attempts before rethrowing.
Including a log statement can help debug issues when resets are triggered during error recovery.
856-862: Preserve the full stack trace.
Returning only the error message drops valuable debugging information. Consider wrapping or re-emitting the original exception.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/appsmith-git/src/main/java/com/appsmith/git/service/ce/GitExecutorCEImpl.java(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (3)
app/server/appsmith-git/src/main/java/com/appsmith/git/service/ce/GitExecutorCEImpl.java (3)
629-637: Confirm user intent before discarding uncommitted changes.
Forcefully resetting to the last commit may cause data loss. Consider prompting or stashing changes if this behavior is unintended.
1031-1043: Recheck behavior of forced reset.
This block discards any uncommitted changes after checking merge conflicts. Ensure this aligns with user expectations to avoid inadvertent data loss.
1124-1129: Ensure exception handling for new method signature.
The updated throws clause means callers must handleGitAPIExceptionandIOException. Double-check error-handling logic throughout the codebase.
3 participants
Description
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Git"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/14106311517
Commit: cef9ab7
Cypress dashboard.
Tags: @tag.Git
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/Apps/ImportExportForkApplication_spec.js
- cypress/e2e/Regression/ClientSide/BugTests/CatchBlock_Spec.ts
- cypress/e2e/Regression/ClientSide/BugTests/DS_Bug28985_spec.ts
- cypress/e2e/Regression/ClientSide/BugTests/GraphQL_Binding_Bug16702_Spec.ts
- cypress/e2e/Regression/ClientSide/Git/GitAutocommit_spec.ts
- cypress/e2e/Regression/ClientSide/Git/GitDiscardChange/DiscardChanges_spec.js
- cypress/e2e/Regression/ClientSide/Git/GitImport/GitImport_spec.js
- cypress/e2e/Regression/ClientSide/Git/GitSync/GitSyncGitBugs_spec.js
- cypress/e2e/Regression/ClientSide/Templates/ForkTemplateToGitConnectedApp.js
- cypress/e2e/Regression/ClientSide/Widgets/Migration_Spec.js
- cypress/e2e/Regression/ServerSide/ApiTests/API_MultiPart_Spec.ts
- cypress/e2e/Regression/ServerSide/ApiTests/API_TestExecuteWithDynamicBindingInUrl_spec.ts
- cypress/e2e/Regression/ServerSide/OnLoadTests/JSOnLoad2_Spec.ts
- cypress/e2e/Regression/ServerSide/Params/PassingParams_Spec.ts
List of identified flaky tests.Thu, 27 Mar 2025 15:43:27 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Bug Fixes
Refactor