chore: add feature flag to the git reset optimisation#39988
chore: add feature flag to the git reset optimisation#39988sondermanish merged 3 commits intoreleasefrom
Conversation
WalkthroughThis pull request adds a new boolean parameter, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GitService
participant FeatureFlagService
participant GitHandler
Client->>GitService: Call saveArtifactToGitRepo(..., keepWorkingDirChanges)
GitService->>FeatureFlagService: Check release_git_reset_optimization_enabled
FeatureFlagService-->>GitService: Return flagStatus
GitService->>GitHandler: Call resetToLastCommit(..., keepWorkingDirChanges = flagStatus)
GitHandler-->>GitService: Return commit result
GitService-->>Client: Return artifact path
Possibly related PRs
Suggested labels
Poem
🪧 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
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/14175413881. |
![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 (4)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/FileInterface.java (1)
38-39: Updated method signature with keepWorkingDirChanges parameter.The
saveArtifactToGitRepomethod now includes a boolean parameter to control whether working directory changes should be preserved, supporting the new feature flag functionality.Consider updating the method's JavaDoc to describe the new parameter and its purpose.
/** * This method is use to store the serialised application to git repo, directory path structure we are going to follow : * ./container-volumes/git-repo/workspaceId/defaultApplicationId/repoName/{application_data} * @param baseRepoSuffix path suffix used to create a repo path * @param gitResourceMap resource map containing the application structure * @param branchName name of the branch + * @param keepWorkingDirChanges whether to preserve working directory changes during git operations * @return Path to where the application is stored */app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/handler/FSGitHandler.java (2)
120-125: Consider clarifying the new parameterAdd brief Javadoc detailing how
keepWorkingDirChangesaffects pull behavior.
241-241: Rebase now supports keepWorkingDirChangesDouble-check that rebase steps align with user expectations for preserving local edits.
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java (1)
572-579: Adding keepWorkingDirChanges parameterThis aligns well with the new feature. Include doc comments so users know when to preserve local changes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java(1 hunks)app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java(10 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/enums/FeatureFlagEnum.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/FileInterface.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/handler/FSGitHandler.java(6 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCECompatibleImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java(8 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/CommonGitFileUtils.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java(6 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/git/ops/GitDiscardTests.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/ExchangeJsonConversionTests.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (1)
Learnt from: abhvsn
PR: appsmithorg/appsmith#39171
File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCEImpl.java:42-44
Timestamp: 2025-03-26T14:08:15.429Z
Learning: The reactive transition for helper methods consuming @FeatureFlagged annotations will be handled in a separate PR to maintain focused changes and proper separation of concerns.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (33)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/enums/FeatureFlagEnum.java (1)
19-22: Well-documented feature flag addition.The documentation clearly communicates the purpose of this feature flag for git reset optimization.
app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/ExchangeJsonConversionTests.java (1)
130-132: Updated mock to match new method signature.The
resetToLastCommitmethod mock now accepts a boolean parameter for keeping working directory changes, which aligns with the feature flag addition.app/server/appsmith-server/src/test/java/com/appsmith/server/git/ops/GitDiscardTests.java (2)
205-207: Updated rebaseBranch mock signature.The mock's signature now includes the boolean parameter for keeping working directory changes.
260-262: Updated getStatus mock signature.The mock's signature now includes the boolean parameter for keeping working directory changes.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (6)
7-7: Dependencies appropriately added for the feature flag implementation.The necessary imports and service dependency for the feature flag functionality have been properly added to the class.
Also applies to: 42-42, 109-109
688-693: Feature flag integration for Git merge operation looks good.The code now checks the
release_git_reset_optimization_enabledfeature flag before performing the merge operation, allowing conditional preservation of working directory changes.
702-711: Feature flag integration for branch merge check looks good.Similar to the merge operation, the code now conditionally preserves working directory changes when checking if branches can be merged, based on the feature flag.
726-733: Feature flag integration for recreating artifacts looks good.The rebase operation now conditionally preserves working directory changes based on the feature flag state, which is consistent with the pattern applied to other Git operations.
742-751: Feature flag integration for Git status operation looks good.The getStatus operation now conditionally preserves working directory changes based on the feature flag state.
851-882: Feature flag integration for pull operation looks good.The pull artifact operation now conditionally preserves working directory changes based on the feature flag state. The error handling remains consistent with the previous implementation.
app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java (1)
249-251: Method signature update for saveArtifactToGitRepo is properly implemented.The method now accepts a
keepWorkingDirChangesparameter which is correctly passed to the underlyingresetToLastCommitmethod. This change is consistent with the feature flag pattern being implemented across the codebase.Also applies to: 257-257
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/CommonGitFileUtils.java (1)
13-13: Constructor properly updated to inject FeatureFlagService.The constructor and super call correctly include the newly added FeatureFlagService parameter. This change is consistent with the feature flag implementation pattern.
Also applies to: 35-48
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (2)
4-4: FeatureFlagService dependency properly added to the base class.The necessary imports, field, and constructor parameter have been properly added for the feature flag functionality.
Also applies to: 40-40, 111-111, 123-135
179-191: Feature flag integration for saving artifacts looks good.The method now checks the
release_git_reset_optimization_enabledfeature flag before saving artifacts, and passes the flag value to the underlying method. Error handling is appropriately maintained.app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceImpl.java (3)
19-19: Adequate importNo issues with importing
FeatureFlagService.
56-57: Add a docstring for the new parameterPlease consider documenting how
featureFlagServiceis intended to be used or if it’s reserved for upcoming updates.
80-81: Constructor chain updatedPassing
featureFlagServiceto the super constructor looks consistent with the feature-flag workflow.app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCECompatibleImpl.java (3)
19-19: New import for feature flagsImporting
FeatureFlagServicealigns with the feature-flag approach.
56-57: Added new parameterEnsure that tests verify usage of
featureFlagServicein any relevant logic paths.
80-81: Constructor chainingForwarding the feature flag service to the superclass is consistent with the pattern used elsewhere.
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/handler/FSGitHandler.java (4)
161-161: Method signature updatedEnsure all callers pass the correct
keepWorkingDirChangesvalue to avoid unexpected resets.
171-172: Merge operation extendedThe inclusion of
keepWorkingDirChangeshere maintains API consistency with related methods.
200-201: "isMergeBranch" parameter additionMake sure tests confirm the behavior when
keepWorkingDirChangesis true or false.
211-212: Reset method extended
keepWorkingDirChangescan significantly alter the workflow. Validate its usage thoroughly.app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java (9)
19-19: FeatureFlagService importNo issues with this new import statement.
637-643: Error handling workflowReverting to the last commit only when
keepWorkingDirChangesis false properly preserves local changes.
862-862: New parameter in getStatusEnsure external calls are updated to supply the desired
keepWorkingDirChangesvalue.
1111-1112: New parameter for mergeThe signature looks good for consistent handling of local changes.
1144-1155: Handling merge conflictsSkipping a hard reset when
keepWorkingDirChangesis true is a sensible approach to preserving local changes.
1288-1289: Extended isMergeBranchConsistently applying
keepWorkingDirChangesacross merge checks is good for uniform behavior.
1340-1341: Conditional skipReturning early if
keepWorkingDirChangesis set avoids unintended overwrites.
1361-1362: Another skip revert scenarioThe implementation cleanly respects the user’s choice to keep local edits.
1516-1551: New rebase flowRebase logic now respects local changes. Confirm test coverage ensures consistent application behavior.
|
Deploy-Preview-URL: https://ce-39988.dp.appsmith.com |
subrata71
left a comment
subrata71
left a comment
There was a problem hiding this comment.
What are the scenarios that require a Git reset, regardless of the feature flag's state?
|
|
||
| // Remove modified changes from current branch so that checkout to other branches | ||
| // will be clean | ||
| if (!status.isClean() && !keepWorkingDirChanges) { |
There was a problem hiding this comment.
Isn't the status "not clean" enough to decide to do a git reset here? If not, can you please explain?
| return resetToLastCommit(repoSuffix, destinationBranch, keepWorkingDirChanges) | ||
| .thenReturn(error.getMessage()); | ||
| } catch (Exception e) { | ||
| log.error("Error while hard resetting to latest commit {0}", e); |
There was a problem hiding this comment.
This is an incorrect format for logging using Slf4j. Please check. I think the correct way would be to do something like this:
log.error("Error while hard resetting to latest commit", e);
There was a problem hiding this comment.
@dvj1988 Understood. SInce the formatting is wrong it won't print what is expected.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (2)
736-744: Consider adding Javadoc to explain the purpose of keepWorkingDirChanges parameter.While the implementation is correct, adding documentation would help explain the purpose of this parameter and how it affects git operations when the feature flag is enabled or disabled.
@Override public Mono<? extends ArtifactExchangeJson> recreateArtifactJsonFromLastCommit( ArtifactJsonTransformationDTO jsonTransformationDTO) { String workspaceId = jsonTransformationDTO.getWorkspaceId(); String baseArtifactId = jsonTransformationDTO.getBaseArtifactId(); String repoName = jsonTransformationDTO.getRepoName(); String refName = jsonTransformationDTO.getRefName(); ArtifactType artifactType = jsonTransformationDTO.getArtifactType(); GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); Path repoSuffix = gitArtifactHelper.getRepoSuffixPath(workspaceId, baseArtifactId, repoName); + // Check if git reset optimization is enabled to determine if working directory changes should be preserved Mono<Boolean> keepWorkingDirChangesMono = featureFlagService.check(FeatureFlagEnum.release_git_reset_optimization_enabled);
861-892: Consider extracting repeated feature flag check to a helper method.This feature flag check is now repeated in multiple methods. Consider creating a private helper method to reduce code duplication.
+ /** + * Gets the value of the git reset optimization feature flag. + * When enabled, working directory changes are preserved during git operations. + * + * @return Mono<Boolean> with the feature flag value + */ + private Mono<Boolean> getKeepWorkingDirChangesFlagValue() { + return featureFlagService.check(FeatureFlagEnum.release_git_reset_optimization_enabled); + } + @Override public Mono<MergeStatusDTO> pullArtifact( ArtifactJsonTransformationDTO jsonTransformationDTO, GitArtifactMetadata baseMetadata) { GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(jsonTransformationDTO.getArtifactType()); Path repoSuffix = gitArtifactHelper.getRepoSuffixPath( jsonTransformationDTO.getWorkspaceId(), jsonTransformationDTO.getBaseArtifactId(), jsonTransformationDTO.getRepoName()); String branchName = jsonTransformationDTO.getRefName(); - Mono<Boolean> keepWorkingDirChangesMono = - featureFlagService.check(FeatureFlagEnum.release_git_reset_optimization_enabled); + Mono<Boolean> keepWorkingDirChangesMono = getKeepWorkingDirChangesFlagValue();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java(10 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/handler/FSGitHandler.java(6 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/handler/FSGitHandler.java
- app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java
🧰 Additional context used
🧠 Learnings (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (1)
Learnt from: abhvsn
PR: appsmithorg/appsmith#39171
File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCEImpl.java:42-44
Timestamp: 2025-03-26T14:08:15.429Z
Learning: The reactive transition for helper methods consuming @FeatureFlagged annotations will be handled in a separate PR to maintain focused changes and proper separation of concerns.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (5)
7-7: Clean implementation of feature flag service dependency.The new imports and dependency injection for the feature flag service are well-structured and follow the existing dependency injection pattern in this class.
Also applies to: 42-42, 109-109
698-703: Good implementation of feature flag in mergeBranches method.The feature flag check is properly integrated with the reactive flow using flatMap to control the keepWorkingDirChanges parameter in the mergeBranch call.
712-721: Consistent implementation in isBranchMergable method.The feature flag check follows the same pattern as in other methods, maintaining consistency throughout the codebase.
752-761: Good implementation of feature flag in getStatus method.The feature flag check is properly integrated with the reactive flow, consistent with other methods.
1-893:Details
❓ Verification inconclusive
Verify FSGitHandler implementations are updated to support keepWorkingDirChanges.
This PR introduces a new parameter to several FSGitHandler methods. Please ensure that the implementation classes properly handle this parameter to avoid unexpected behavior.
🏁 Script executed:
#!/bin/bash # Check FSGitHandler implementation for changed method signatures # Methods that now include the keepWorkingDirChanges parameter methods=("pullArtifactWithoutCheckout" "getStatus" "mergeBranch" "isMergeBranch" "resetToLastCommit" "rebaseBranch") # Find FSGitHandler implementations and check for those methods echo "Checking for FSGitHandler implementations and relevant methods:" echo "--------------------------------------------------------------" ast-grep --pattern 'class $CLASS_NAME implements FSGitHandler' # Search for method implementations for method in "${methods[@]}"; do echo -e "\nChecking implementations of $method that use keepWorkingDirChanges parameter:" rg -A 3 "public\s+(?:Mono<[^>]+>|[^(]+)\s+$method\s*\([^)]*boolean\s+keepWorkingDirChanges[^)]*\)" --type java doneLength of output: 4432
FSGitHandler Parameter Update Verification
We've confirmed that in
FSGitHandlerCEImplthe methodsgetStatus,resetToLastCommit, andrebaseBranchnow include and handle the newkeepWorkingDirChangesparameter. However, our search did not locate updated signatures for the following methods—which are still being invoked with the parameter in the service layer:
pullArtifactWithoutCheckoutmergeBranchisMergeBranchPlease review these methods to ensure they either accept the new parameter or correctly propagate it through their implementations, preventing any unexpected git behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java (7)
637-643: Enhance error handling & reduce duplicationIt might be helpful to log the original error before returning, and unify this fallback reset logic into a shared helper method to minimize repetitive code blocks.
862-862: Add param documentationConsider including a short note in the method’s Javadoc to clarify the intended usage of the
keepWorkingDirChangesparameter.
916-925: Prevent accidental data lossUnconditionally resetting the repository may discard partially staged changes. Consider prompting the user or adding an additional conditional check before performing a hard reset.
1144-1155: Extract repeated reset-on-error logicThis fallback reset pattern after errors appears multiple times. Consider extracting it into a well-documented helper method for consistency and easier maintenance.
1338-1354: Consolidate fallback logicThis segment resembles the error handling in other merge methods. Factoring out a common approach would reduce duplication and centralize handling of resets vs. preservation of changes.
1361-1371: Log potential data discardWhen
keepWorkingDirChangesis false, consider logging a warning before the reset to inform about the possibility of losing uncommitted changes.
1528-1563: Improve rebase error messagingWhen a rebase fails, consider adding more context or user-friendly instructions in the thrown exception, especially if
keepWorkingDirChangesis false and local changes are forcibly discarded.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (4)
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java (4)
573-579: Document thekeepWorkingDirChangesparameterAdd a short Javadoc explanation for the new parameter, clarifying how local uncommitted changes are handled when set to
trueorfalse.
1111-1112: Ensure testing for the new parameterVerify that the scenarios involving
keepWorkingDirChangesare well-covered by unit or integration tests, ensuring both “true” and “false” cases are exercised.
1287-1288: Expand test coverage for merge checksConfirm that test suites cover the new
keepWorkingDirChangesparameter inisMergeBranch, particularly cases where conflicts or errors occur.
1489-1499: Verify the double-reset logicThis flow attempts a reset, checks out a branch, then resets again if
keepWorkingDirChangesis false. Please verify that this sequence is intentional and doesn't introduce unexpected behavior for end users.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/14188686987. |
|
Deploy-Preview-URL: https://ce-39988.dp.appsmith.com |
) ## Description Toggle the [remove extra git resets changes](appsmithorg#39758) in the new git apis behind a feature flag `release_git_reset_optimization_enabled`. Fixes #`Issue Number` _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/14188261678> > Commit: 5f3ce61 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=14188261678&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git` > Spec: > <hr>Tue, 01 Apr 2025 06:48:38 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 feature flag for enhanced Git reset optimization, enabling optional preservation of local changes during Git operations. - **Refactor** - Updated Git workflows to offer more flexible handling of uncommitted changes and improved error management. - **Tests** - Adjusted test scenarios to validate the refined Git behavior and ensure consistent operation under the new settings. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
Description
Toggle the remove extra git resets changes in the new git apis behind a feature flag
release_git_reset_optimization_enabled.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
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/14188261678
Commit: 5f3ce61
Cypress dashboard.
Tags:
@tag.GitSpec:
Tue, 01 Apr 2025 06:48:38 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Refactor
Tests