chore: FS layer has additional reset responsibility before reconstruction #39989
chore: FS layer has additional reset responsibility before reconstruction #39989sondermanish merged 1 commit intoreleasefrom
Conversation
WalkthroughThe pull request introduces a new public method Changes
Sequence Diagram(s)sequenceDiagram
participant GitFS as GitFSServiceCEImpl
participant FSGit as FSGitHandlerCEImpl
participant Helper as GitArtifactHelper
participant Common as CommonGitFileUtils
GitFS->>Helper: Retrieve helper based on artifact type
Helper-->>GitFS: Return helper & repoSuffix details
GitFS->>FSGit: resetToLastCommit(repoSuffix)
FSGit-->>GitFS: Return Mono<Boolean>
GitFS->>Common: constructArtifactExchangeJsonFromGitRepositoryWithAnalytics(dto)
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.31.1)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java✨ 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
Bug
github-actions
bot
added
skip-changelog
github-actions
bot
added
Bug
![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
🧹 Nitpick comments (1)
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java (1)
1417-1428: Fix typo in Javadoc and consider clarifying error handling.
- The word "successfull" should be "successful" in the Javadoc.
- Currently, errors are suppressed and return false. Consider clarifying in the doc or handling errors so the caller is aware of the failure details.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/handler/FSGitHandler.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)
872-872: Grammar correction looks goodFixing "is already exists" to "already exists" improves the error message clarity.
867-873: Improved Error Message Clarity in createReference Method
The error message has been updated to:
“either ref name already exists or it doesn't meet naming criteria, or the artifact is not in a publishable state”
This change corrects the grammatical error from the previous version and improves overall readability. Please verify that this message still accurately conveys the failure conditions in your ref creation workflow.app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/handler/FSGitHandler.java (1)
206-211: New method signature adds important functionalityThis new overloaded method allows resetting to the last commit on the current branch without needing to specify a branch name explicitly, which is useful for the artifact reconstruction flow mentioned in the PR description.
| GitArtifactHelper<?> gitArtifactHelper = | ||
| gitArtifactHelperResolver.getArtifactHelper(artifactJsonTransformationDTO.getArtifactType()); | ||
|
|
||
| Path repoSuffix = gitArtifactHelper.getRepoSuffixPath( | ||
| artifactJsonTransformationDTO.getWorkspaceId(), | ||
| artifactJsonTransformationDTO.getBaseArtifactId(), | ||
| artifactJsonTransformationDTO.getRepoName()); | ||
|
|
||
| return fsGitHandler | ||
| .resetToLastCommit(repoSuffix) | ||
| .then(commonGitFileUtils.constructArtifactExchangeJsonFromGitRepositoryWithAnalytics( | ||
| artifactJsonTransformationDTO)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle possible reset failures.
resetToLastCommit(repoSuffix) may fail and return false. Consider explicitly handling or logging this result before proceeding to construct the artifact JSON to avoid potential inconsistencies.
…tion (appsmithorg#39989) ## Description ### This helps in resetting the file system to last commit in the following flows - Import - Checkout ref (Branch/tag) - pull - merge - discard Fixes appsmithorg#39934 ## 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/14176842029> > Commit: a8202cb > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=14176842029&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git` > Spec: > <hr>Mon, 31 Mar 2025 17:39:11 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 - **New Features** - Introduced a repository reset option allowing users to revert to the latest commit on the current branch without needing to specify extra details. - Enhanced the artifact restoration process by applying this reset step to ensure a consistent repository state before rebuilding data. - **Bug Fixes** - Updated error notifications during reference creation for improved clarity. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
This helps in resetting the file system to last commit in the following flows
Fixes #39934
Automation
/ok-to-test tags="@tag.Git"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/14176842029
Commit: a8202cb
Cypress dashboard.
Tags:
@tag.GitSpec:
Mon, 31 Mar 2025 17:39:11 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit