-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: added interface changes to the server #38258
Conversation
WalkthroughThis pull request introduces enhanced Git reference management capabilities across multiple Appsmith server components. Key additions include a new method for creating and checking out Git references (branches and tags) in the Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (1)
Line range hint
666-675
: Fix incomplete implementation in listReferences methodThe method has several issues:
- Missing return statement in the BRANCH case
- Always returns an empty list regardless of the branch case
- Incomplete implementation for tag references
Apply this fix:
@Override public Mono<List<String>> listReferences( ArtifactJsonTransformationDTO artifactJsonTransformationDTO, Boolean checkRemoteReferences, RefType refType) { if (RefType.BRANCH.equals(refType)) { - listBranches(artifactJsonTransformationDTO, checkRemoteReferences); + return listBranches(artifactJsonTransformationDTO, checkRemoteReferences); } // TODO: include ref type for tags in fsGit Handler return Mono.just(List.of()); }
🧹 Nitpick comments (4)
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java (1)
361-374
: Check for message usage when tagging.
You are correctly setting the tag message. Verify that you handle empty or null messages gracefully, if needed.app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)
537-546
: Handle unclean status more gracefully.
The error message is generic. If possible, provide details as to why the status was unclean for faster troubleshooting.
568-571
: Verify publish logic post reference creation.
Publishing can sometimes be asynchronous or partial. Double-check that the artifact is in a stable state if post-publish fails.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitArtifactHelperCE.java (1)
72-73
: Publish logic after reference creation.
This added method is straightforward. Confirm if you need to handle any exceptions within the publication flow for newly created references.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java
(2 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/dtos/GitRefDTO.java
(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/handler/FSGitHandler.java
(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/GitApplicationHelperCEImpl.java
(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java
(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java
(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java
(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitArtifactHelperCE.java
(2 hunks)
👮 Files not reviewed due to content moderation or server errors (3)
- app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/handler/FSGitHandler.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/GitApplicationHelperCEImpl.java
🔇 Additional comments (11)
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java (4)
8-8
: No issue found for the new import.
12-12
: Import usage check.
349-359
: Validate synergy between branch name & tracking config.
If the branch name or upstream config is incorrect, this could break business logic. The logic here appears sound, but ensure error handling covers invalid or empty branch names.
376-402
: Ensure thorough error handling for reference creation.
Consider logging or throwing a more explicit exception if neither branch nor tag is identified. Also confirm that you handle rewrite scenarios, e.g., reassigning an existing tag name.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (3)
525-527
: Single responsibility for the validation method.
This call to "isValidationForRefCreationComplete" is well-placed. Ensure it’s unit-tested to confirm ref-quality checks.
553-554
: Consider fallback scenario or conflict resolution.
When creating the Git reference, handle collisions with existing references or further conflicts.
591-604
: Attentive check for unclean status on tags.
The method returns TRUE on success, but be sure your usage logs details if unclean.
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/dtos/GitRefDTO.java (1)
17-21
: Great addition for tag messages.
Ensure the field is optional for branch references and not required unless explicitly used for tagging.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitArtifactHelperCE.java (1)
3-3
: No issues in new import statement.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (2)
5-5
: LGTM!
The GitRefDTO import is correctly placed and necessary for the implementation changes.
Line range hint 666-675
: LGTM! Clean implementation of reference creation
The method properly delegates to the FSGitHandler and uses the new GitRefDTO parameter effectively.
repoSuffix); | ||
|
||
if (RefType.TAG.equals(refType)) { | ||
return createAndCheckoutTag(git, gitRefDTO); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why checkout to tag at all as a complete function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't that cause inconsistency? I'm under the assumption that we are going to land on that tag. (It doesn't make any difference however when we checkout a tag then we would need to do it in FS as well no, so for consistency I've kept that.
Failed server tests
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java (2)
367-393
: Consider enhancing error handling in createAndCheckoutReference.The implementation looks good, but consider adding specific error handling for common Git operations failures.
@Override public Mono<String> createAndCheckoutReference(Path repoSuffix, GitRefDTO gitRefDTO) { RefType refType = gitRefDTO.getRefType(); String refName = gitRefDTO.getRefName(); return Mono.using( () -> Git.open(createRepoPath(repoSuffix).toFile()), git -> Mono.fromCallable(() -> { log.info( "{} : Creating reference of type {} and name {} for the repo {}", Thread.currentThread().getName(), refType.name(), refName, repoSuffix); if (RefType.TAG.equals(refType)) { return createTag(git, gitRefDTO); } return createAndCheckoutBranch(git, gitRefDTO); }) + .onErrorResume(GitAPIException.class, e -> { + log.error("Git operation failed: {}", e.getMessage()); + return Mono.error(new Exception("Failed to create reference: " + e.getMessage())); + }) .timeout(Duration.ofMillis(Constraint.TIMEOUT_MILLIS)) .name(GitSpan.FS_CREATE_BRANCH) .tap(Micrometer.observation(observationRegistry)), Git::close) .subscribeOn(scheduler); }
375-381
: Enhance logging for better observability.Consider adding debug logs for operation success/failure states.
log.info( "{} : Creating reference of type {} and name {} for the repo {}", Thread.currentThread().getName(), refType.name(), refName, repoSuffix); +log.debug("Starting {} operation for reference {}", refType, refName);
📜 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/handler/ce/FSGitHandlerCEImpl.java
(2 hunks)
🔇 Additional comments (1)
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java (1)
8-8
: LGTM: Required imports added for Git reference management.
Also applies to: 12-12
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (2)
692-695
: Tie lock release failures to additional logging.
Releasing file locks on error is an excellent design choice. However, log the result of releaseLockMono for better visibility in case of lock-release failures.
697-700
: Provide user-friendly instructions for current-branch deletions.
The exception message is correct. You might enhance user guidance (e.g., specify a different branch first) so they know how to resolve this situation.app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (4)
422-429
: Consider adding error handling for checkout failuresThe checkout operation could fail for various reasons (e.g., conflicts, network issues). Consider adding specific error handling to provide better feedback to users.
checkedOutArtifactMono = gitHandlingService .checkoutArtifact(jsonTransformationDTO) + .onErrorResume(error -> { + log.error("Error during checkout: {}", error.getMessage()); + return Mono.error(new AppsmithException( + AppsmithError.GIT_ACTION_FAILED, + "checkout", + error.getMessage())); + }) .flatMap(isCheckedOut -> gitArtifactHelper.getArtifactByBaseIdAndBranchName(
Line range hint
529-550
: Enhance validation logic with early returnsThe validation and preparation logic is well-structured but could be more concise with early returns.
-return refCreationValidationMono.flatMap(isOkayToProceed -> { - if (!TRUE.equals(isOkayToProceed)) { - return Mono.error(new AppsmithException( - AppsmithError.GIT_ACTION_FAILED, - "ref creation", - "status unclean")); - } - return Mono.zip(newArtifactFromSourceMono, artifactExchangeJsonMono); -}); +return refCreationValidationMono + .filter(TRUE::equals) + .switchIfEmpty(Mono.error(new AppsmithException( + AppsmithError.GIT_ACTION_FAILED, + "ref creation", + "status unclean"))) + .then(Mono.zip(newArtifactFromSourceMono, artifactExchangeJsonMono));
572-577
: Consider adding transaction boundaryThe post-reference creation operations (publish and discard) should ideally be atomic. Consider wrapping them in a transaction.
1424-1432
: Consider adding timeout for Git status operationsGit status operations can be time-consuming. Consider adding a timeout to prevent long-running operations.
return Mono.zip(prepareForStatus, fetchRemoteMono).flatMap(tuple2 -> { return gitHandlingService .getStatus(jsonTransformationDTO) + .timeout(Duration.ofSeconds(30)) + .onErrorResume(TimeoutException.class, ex -> + Mono.error(new AppsmithException(AppsmithError.GIT_EXECUTION_TIMEOUT))) .flatMap(gitStatusDTO -> { return gitRedisUtils .releaseFileLock(baseArtifactId, isFileLock) .thenReturn(gitStatusDTO); }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java
(8 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java
(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java
(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java
🔇 Additional comments (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (3)
5-5
: Import statement looks good.
No issues with introducing “GitRefDTO” as part of this file's import.
Line range hint 666-675
: Ensure concurrency handling for duplicate references.
While invoking createAndCheckoutReference, consider verifying that no other concurrent operation is creating the same reference to avoid potential conflicts.
708-721
: Validate artifact type vs. ref name usage.
Since this method treats tags and branches identically, verify that a user referencing a tag name can be checked out successfully without confusion. Otherwise, consider an explicit distinction for improved clarity.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (3)
557-558
: LGTM!
The Git reference creation implementation is correct and follows reactive programming patterns.
582-588
: LGTM!
The file lock release and analytics implementation is correct and follows best practices.
1686-1691
: LGTM!
The Git discard changes lock acquisition is implemented correctly with proper error handling.
|
||
return releaseLockMono.then(Mono.error(new AppsmithException( | ||
AppsmithError.GIT_ACTION_FAILED, "delete branch", throwable.getMessage()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consolidate error-handling branches if possible.
Consider merging the logic for the generic branch-deletion failure with the current-branch error path for consistency and fewer code branches.
protected Mono<Boolean> isValidationForRefCreationComplete( | ||
Artifact baseArtifact, Artifact parentArtifact, GitType gitType, RefType refType) { | ||
if (RefType.BRANCH.equals(refType)) { | ||
return Mono.just(TRUE); | ||
} | ||
|
||
return getStatus(baseArtifact, parentArtifact, false, true, gitType).map(gitStatusDTO -> { | ||
if (!Boolean.TRUE.equals(gitStatusDTO.getIsClean())) { | ||
return FALSE; | ||
} | ||
|
||
return TRUE; | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider adding validation for tag name format
The reference creation validation method should also validate the tag name format when RefType is TAG.
protected Mono<Boolean> isValidationForRefCreationComplete(
Artifact baseArtifact, Artifact parentArtifact, GitType gitType, RefType refType) {
if (RefType.BRANCH.equals(refType)) {
return Mono.just(TRUE);
}
+ if (RefType.TAG.equals(refType)) {
+ String tagName = parentArtifact.getGitArtifactMetadata().getRefName();
+ if (!isValidTagName(tagName)) {
+ return Mono.just(FALSE);
+ }
+ }
return getStatus(baseArtifact, parentArtifact, false, true, gitType).map(gitStatusDTO -> {
Committable suggestion skipped: line range outside the PR's diff.
.flatMap(branchedArtifact -> { | ||
return gitRedisUtils | ||
.releaseFileLock( | ||
branchedArtifact.getGitArtifactMetadata().getDefaultArtifactId(), TRUE) | ||
.then(gitAnalyticsUtils.addAnalyticsForGitOperation( | ||
AnalyticsEvents.GIT_DISCARD_CHANGES, branchedArtifact, null)); | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider adding retry mechanism for lock release
Lock release is critical for preventing deadlocks. Consider adding a retry mechanism with exponential backoff.
.flatMap(branchedArtifact -> {
return gitRedisUtils
.releaseFileLock(
branchedArtifact.getGitArtifactMetadata().getDefaultArtifactId(), TRUE)
+ .retryWhen(Retry.backoff(3, Duration.ofMillis(100))
+ .filter(throwable -> throwable instanceof LockAcquisitionException))
.then(gitAnalyticsUtils.addAnalyticsForGitOperation(
AnalyticsEvents.GIT_DISCARD_CHANGES, branchedArtifact, null));
})
Committable suggestion skipped: line range outside the PR's diff.
@@ -575,13 +579,13 @@ public Mono<? extends Artifact> createReference( | |||
discardChanges(parentArtifact.getId(), artifactType, gitType)); | |||
}); | |||
}) | |||
.flatMap(newImportedArtifact -> gitAnalyticsUtils | |||
.addAnalyticsForGitOperation( | |||
.flatMap(newImportedArtifact -> gitRedisUtils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will we release in case of errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No release in chain
@@ -681,7 +685,9 @@ protected Mono<? extends Artifact> deleteGitReference( | |||
|
|||
return gitHandlingService | |||
.deleteGitReference(jsonTransformationDTO) | |||
.doFinally(signalType -> gitRedisUtils.releaseFileLock(baseArtifactId, TRUE)) | |||
.flatMap(isReferenceDeleted -> gitRedisUtils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same error handling question for all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No release in chain
Failed server tests
|
.doFinally(signalType -> gitRedisUtils.releaseFileLock(baseArtifactId, addFileLock)) | ||
return acquireFileLock | ||
.then(checkedOutArtifactMono) | ||
.flatMap(checkedOutArtifact -> gitRedisUtils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sondermanish doesn't look like this one has a release in the chain
}) | ||
.doFinally(signalType -> gitRedisUtils.releaseFileLock(baseArtifactId, isFileLock)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No release in chain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of those have it Manish, gotta fix :)
.addAnalyticsForGitOperation(AnalyticsEvents.GIT_DISCARD_CHANGES, branchedArtifact, null) | ||
.doFinally(signalType -> gitRedisUtils.releaseFileLock( | ||
branchedArtifact.getGitArtifactMetadata().getDefaultArtifactId(), TRUE))) | ||
.flatMap(branchedArtifact -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No release in chain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops approved by mistake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting on changes, removing from queue.
There was a problem hiding this 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
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)
1219-1226
: Consider handling specific Git commit errorsThe error handling could be more granular to provide better user feedback for different failure scenarios.
- return gitRedisUtils.releaseFileLock(baseArtifact.getId(), TRUE) - .then(gitAnalyticsUtils + String errorMessage = error.getMessage(); + if (error instanceof org.eclipse.jgit.api.errors.EmptyCommitException) { + errorMessage = "No changes to commit"; + } else if (error instanceof org.eclipse.jgit.api.errors.UnmergedPathsException) { + errorMessage = "Please resolve conflicts before committing"; + } + return gitRedisUtils.releaseFileLock(baseArtifact.getId(), TRUE) + .then(gitAnalyticsUtils
1696-1699
: Enhance error messages in discardChanges operationWhile the error handling is good, the error messages could be more descriptive to help users understand and resolve issues.
- return Mono.error(new AppsmithException(AppsmithError.GIT_ACTION_FAILED, "checkout")); + String errorContext = error instanceof org.eclipse.jgit.api.errors.CheckoutConflictException + ? "Unable to discard changes due to conflicts" + : "Failed to discard changes"; + return Mono.error(new AppsmithException( + AppsmithError.GIT_ACTION_FAILED, + "discard", + errorContext + ": " + error.getMessage()));Also applies to: 1701-1757
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java
(8 hunks)
🔇 Additional comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (1)
477-482
: LGTM! Clean implementation of reference creation
The code follows good practices by:
- Using tuples for related data
- Proper error handling through monos
- Clear separation of concerns
checkedOutArtifactMono = gitHandlingService | ||
.checkoutArtifact(jsonTransformationDTO) | ||
.flatMap(isCheckedOut -> gitArtifactHelper.getArtifactByBaseIdAndBranchName( | ||
baseArtifactId, finalRefName, gitArtifactHelper.getArtifactReadPermission())) | ||
.flatMap(artifact -> gitAnalyticsUtils.addAnalyticsForGitOperation( | ||
AnalyticsEvents.GIT_CHECKOUT_BRANCH, | ||
artifact, | ||
artifact.getGitArtifactMetadata().getIsRepoPrivate()))); | ||
artifact.getGitArtifactMetadata().getIsRepoPrivate())); | ||
} | ||
|
||
return checkedOutArtifactMono | ||
.doFinally(signalType -> gitRedisUtils.releaseFileLock(baseArtifactId, addFileLock)) | ||
return acquireFileLock | ||
.then(checkedOutArtifactMono) | ||
.flatMap(checkedOutArtifact -> gitRedisUtils | ||
.releaseFileLock(baseArtifactId, addFileLock) | ||
.thenReturn(checkedOutArtifact)) | ||
.onErrorResume(error -> { | ||
log.error("An error occurred while checking out the reference. error {}", error.getMessage()); | ||
return gitRedisUtils | ||
.releaseFileLock(baseArtifactId, addFileLock) | ||
.then(Mono.error(error)); | ||
}) | ||
.tag(GitConstants.GitMetricConstants.CHECKOUT_REMOTE, FALSE.toString()) | ||
.name(GitSpan.OPS_CHECKOUT_BRANCH) | ||
.tap(Micrometer.observation(observationRegistry)) | ||
.onErrorResume(Mono::error); | ||
.tap(Micrometer.observation(observationRegistry)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve error handling in checkout operation
The error handling could be more robust by:
- Handling specific Git exceptions (e.g., CheckoutConflictException)
- Ensuring lock release in all scenarios using try-with-resources or similar pattern
- checkedOutArtifactMono = gitHandlingService
- .checkoutArtifact(jsonTransformationDTO)
- .flatMap(isCheckedOut -> gitArtifactHelper.getArtifactByBaseIdAndBranchName(
- baseArtifactId, finalRefName, gitArtifactHelper.getArtifactReadPermission()))
+ checkedOutArtifactMono = Mono.using(
+ () -> baseArtifactId,
+ id -> gitHandlingService
+ .checkoutArtifact(jsonTransformationDTO)
+ .flatMap(isCheckedOut -> gitArtifactHelper.getArtifactByBaseIdAndBranchName(
+ id, finalRefName, gitArtifactHelper.getArtifactReadPermission()))
+ .onErrorResume(e -> {
+ if (e instanceof CheckoutConflictException) {
+ return Mono.error(new AppsmithException(
+ AppsmithError.GIT_ACTION_FAILED,
+ "checkout",
+ "Conflicts detected. Please resolve conflicts and try again."));
+ }
+ return Mono.error(e);
+ }),
+ id -> gitRedisUtils.releaseFileLock(id, addFileLock).subscribe()
+ )
Committable suggestion skipped: line range outside the PR's diff.
## Description - interface layer changes for Git executor Fixes # > [!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/12481281011> > Commit: e510ca6 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12481281011&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git` > Spec: > <hr>Tue, 24 Dec 2024 16:05:39 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 ## Summary by CodeRabbit - **New Features** - Introduced methods for creating and checking out branches and tags in Git. - Added functionality for validating reference creation based on artifact status. - Enhanced artifact publishing with new methods related to reference creation. - Added a method for checking out artifacts based on provided details. - **Bug Fixes** - Improved error handling for Git operations, providing more specific error messages. - **Documentation** - Updated method signatures to reflect new parameters and functionalities. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Description - interface layer changes for Git executor Fixes # > [!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/12481281011> > Commit: e510ca6 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12481281011&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git` > Spec: > <hr>Tue, 24 Dec 2024 16:05:39 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 ## Summary by CodeRabbit - **New Features** - Introduced methods for creating and checking out branches and tags in Git. - Added functionality for validating reference creation based on artifact status. - Enhanced artifact publishing with new methods related to reference creation. - Added a method for checking out artifacts based on provided details. - **Bug Fixes** - Improved error handling for Git operations, providing more specific error messages. - **Documentation** - Updated method signatures to reflect new parameters and functionalities. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Fixes #
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
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12481281011
Commit: e510ca6
Cypress dashboard.
Tags:
@tag.Git
Spec:
Tue, 24 Dec 2024 16:05:39 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation