chore: merge and merge status #38495
Conversation
sondermanish
requested review from
sharat87,
abhvsn,
AnaghHegde and
a team
as code owners
WalkthroughThis pull request introduces enhancements to the Git resource management and migration functionality across multiple server components. The changes primarily focus on expanding methods related to Git operations, including new methods for constructing Git resource maps, handling branch merges, and improving JSON schema migration processes. The modifications span several classes and interfaces, introducing more flexible reference handling and expanding the capabilities of Git-related services. Changes
Assessment against linked issues
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (29)
🔇 Additional comments (86)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Git Product
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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 3
🧹 Nitpick comments (33)
app/server/appsmith-server/src/test/java/com/appsmith/server/migrations/JsonSchemaMigrationTest.java (1)
42-42: Consider gracefully handling null references in the migration call.Passing multiple
nullarguments can be fragile if future logic changes rely on these parameters. Ensure thatmigrateArtifactExchangeJsonToLatestSchemagracefully handles nulls without side effects.app/server/appsmith-server/src/main/java/com/appsmith/server/applications/imports/ApplicationImportServiceCEImpl.java (1)
653-657: Check redundant assignment.
RefType refTypeis declared at line 653 and assigned at line 657. This is fine, but consider collapsing these assignments if you don’t need the variable as null beforehand.- RefType refType = null; ... - refType = application.getGitArtifactMetadata().getRefType(); + RefType refType = application.getGitArtifactMetadata().getRefType();app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.java (2)
153-165: New merge method
Implementation is straightforward. Consider providing descriptive logs or metrics around potential merge conflict scenarios.
167-177: Verify branch correctness
Implementation looks consistent. Adding further debug logs for missing or invalid branches could assist troubleshooting.app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (2)
630-643: Branch merging
Implementation merges sources into destinations as expected. Consider adding conflict handling or logging.
644-659: Merge status check
Implementation is cohesive. Ensuring robust error handling for unexpected merges or invalid refs would be beneficial.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (2)
560-593: constructArtifactExchangeJsonFromGitRepositoryWithAnalytics method
Implementation is well-structured. Keep an eye on performance overhead when combining analytics.
601-620: Repository reconstruction
Flow is straightforward. Consider logging IO or JSON parsing difficulties for debugging.app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (5)
1146-1149: Commit artifact flow
Step-by-step approach is clear. Double-check concurrency safety for multiple parallel commits.
1397-1397: Remote detachment
Method thoroughly cleans up local repos. Consider logging partial cleanup attempts.
1481-1482: Status comparison
Meaningful naming for both local and remote checks. Handling large repos or timeouts might require future enhancements.
2602-2821: Branch merging logic
Effectively merges source and destination. Consider fallback strategies for conflicts beyond throwing exceptions.
2824-3011: Branch merge checks
Method carefully verifies behind counts for both branches. Adding logs for unclean states would improve clarity.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ArtifactGitFileUtilsCE.java (1)
32-34: Consider providing Javadoc for the new method.
Adding Javadoc forperformJsonMigrationwould improve clarity and maintainability, making it easier for other developers to understand its purpose and usage.app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java (3)
50-52: Branch merging as a new feature.
The newly addedmergeBranchmethod is beneficial for advanced repository management. Consider adding doc comments or usage examples to help the next developer.
65-65: Ensure consistent usage ofcheckoutReference.
This method can be disruptive if not used carefully; consider adding warnings in logs for user clarity.
71-71: Revisit naming.
createReferencemight be more expressive if named to indicate whether it creates local or remote references.app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java (1)
81-81: Include merge conflict policy in doc comments.
Providing an overview of conflict resolution formergeBrancheswill help maintainers.app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/FileInterface.java (1)
53-54: Document repository expectations.
constructGitResourceMapFromGitRepois a core method for retrieving resources. Provide Javadoc clarifying expected path structures and handling for partial/failing states.app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaMigration.java (2)
45-47: Keep parameter descriptions up to date
The Javadoc references the newrefNameandrefType. Ensure all references to "branchName" in the commentary are updated for consistency if not done.
50-54: Document fallback flow for non-application artifacts
Code gracefully falls back ifartifactExchangeJsonis not ofAPPLICATIONtype. Briefly documenting this fallback can be helpful.app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/handler/FSGitHandler.java (1)
138-139: Clarify concurrency approach
Consider documenting whether concurrent merges to the same destination branch are handled or restricted.app/server/appsmith-server/src/test/java/com/appsmith/server/git/ops/GitCommitTests.java (2)
139-139: Update documentation.
This line change is consistent with the newsaveArtifactToLocalRepoNewmethod name. Consider updating any relevant Javadocs or comments referencing the old method name for clarity.
228-228: Handle repository not found cases thoroughly.
Although you are mocking aRepositoryNotFoundException, ensure that error paths for missing local repos are handled gracefully in production.app/server/appsmith-server/src/test/java/com/appsmith/server/git/ops/GitDiscardTests.java (1)
246-246: Prevent potential null pointer issues.
If any of the parameters passed tosaveArtifactToLocalRepoNewcan be null, consider adding defensive checks to avoid unexpected runtime errors.app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/GitFileUtilsTest.java (1)
87-88: Ensure optional parameter usage is clarified.
Here, the new fourth parameter is passed asnull. Consider adding a brief explanation if it’s intentionally left unused for future expansions.app/server/appsmith-server/src/test/java/com/appsmith/server/git/ops/GitConnectTests.java (2)
254-254: Revisit error handling.
Exceptions during cloning must be well-labeled and user-friendly. Current error message is okay, but consider more contextual detail for debug logs.
473-473: Improve debug logs around commit failures.
You’re catchingdefault commit error. Consider logging helpful details about the commit attempt (branch, user, etc.).app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitServiceTest.java (1)
548-548: Add negative test coverage
Consider adding a test scenario where an invalidRefTypeis passed, ensuring the code handles unexpected references.app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java (1)
472-472: Use concise variable naming
In some future expansions, consider a more descriptive name forRefType.branchif multiple ref types are heavily used. Currently fine.app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java (1)
2727-2728: Maintain consistency across additional parameters
Similar to lines 364–365, the added parameters are also null here. Make sure a consistent approach is taken throughout the codebase to handle these optional parameters, and consider adding or updating tests to confirm correctness.app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java (2)
849-855: Use caution with higher log level.
If you want to track merges more visibly, switching fromdebugtoinfois fine. Otherwise, consider whether this increased verbosity is necessary.
981-983: Elevating log messages to info.
This might be too verbose if merges are frequent. Confirm if that extra visibility is desired.
🛑 Comments failed to post (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java (2)
53-55: 🛠️ Refactor suggestion
Add conflict resolution plan.
TheisBranchMergablemethod is essential to detect merge conflicts. Document or log possible conflict file details to assist the user.
77-77: 🛠️ Refactor suggestion
Enhance protection logic.
Consider logging or failing gracefully if a branch cannot be protected or is disallowed.app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java (1)
83-83: 🛠️ Refactor suggestion
Perform additional validations.
isBranchMergableshould confirm that remote references match the local HEAD to avoid unexpected merges.
Failed server tests
|
| RefSpec ref = new RefSpec( | ||
| "refs/heads/" + branchName + ":refs/remotes/origin/" + branchName); | ||
| "refs/heads/" + refName + ":refs/remotes/origin/" + refName); |
There was a problem hiding this comment.
This format is only applicable to branches, not tags. We'll need a separate path for tags
| @@ -1054,6 +1035,20 @@ public Mono<MergeStatusDTO> isMergeBranch(Path repoSuffix, String sourceBranch, | |||
| return Mono.error(e); | |||
| } | |||
| }) | |||
| .onErrorResume(error -> { | |||
| try { | |||
There was a problem hiding this comment.
In case of errors we would need to reset it to last commit, and the method reset to last commit is throwing checked exceptions which needs to be handled.
| .fetchRemoteReferences( | ||
| jsonTransformationDTO, fetchRemoteDTO, baseGitMetadata.getGitAuth()) | ||
| .flatMap(remoteSpecs -> { | ||
| Mono<GitStatusDTO> sourceBranchStatusMono = Mono.defer(() -> getStatus( |
There was a problem hiding this comment.
Nit can we pull these out into variables for readability?
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
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 #37453, #37454
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/12666543761
Commit: ab5e336
Cypress dashboard.
Tags:
@tag.GitSpec:
Wed, 08 Jan 2025 14:49:03 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the concise release notes:
New Features
Improvements
Technical Updates
These changes primarily focus on improving Git integration, providing more flexible reference management, and streamlining the application's version control capabilities.