chore: added status changes #38170
chore: added status changes #38170
Conversation
|
Warning Rate limit exceeded@sondermanish has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces modifications to the Git-related services and configurations in the Appsmith server application. Key changes include updating the Java Runtime Environment configuration for the Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
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
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: 3
🧹 Outside diff range and nitpick comments (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (1)
946-954: Update Javadoc to reflect the correct return typeThe Javadoc mentions that the method returns a
Map, but it actually returns aMono<GitStatusDTO>. Update the documentation for accuracy.Suggested fix:
- * @return Map of json file names which are added, modified, conflicting, removed and the working tree if this is clean + * @return Mono of GitStatusDTO containing the Git status informationapp/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java (1)
38-39: Add Javadoc for the newgetStatusmethodInclude Javadoc comments to improve readability and maintainability.
Suggested addition:
/** * Retrieves the Git status for the specified branched artifact. * * @param branchedArtifactId the ID of the branched artifact * @param compareRemote whether to compare with the remote repository * @param artifactType the type of the artifact * @param gitType the type of the Git service * @return Mono emitting GitStatusDTO containing the status information */ Mono<GitStatusDTO> getStatus( String branchedArtifactId, boolean compareRemote, ArtifactType artifactType, GitType gitType);app/server/appsmith-interfaces/src/main/java/com/appsmith/external/dtos/ce/GitStatusCE_DTO.java (3)
11-11: Address the TODO commentPlease resolve or remove the TODO comment to maintain code clarity.
Line range hint
14-79: Declare class fields asprivateTo encapsulate the data properly, declare the class fields as
private.Suggested change:
- Set<String> modified; + private Set<String> modified; - Set<String> added; + private Set<String> added; - // ... and so on for all fieldsApply this change to all fields in the class.
Line range hint
57-73: Use Javadoc@deprecatedtags for deprecated fieldsTo provide more context, add Javadoc
@deprecatedannotations alongside the@Deprecatedannotation.Suggested addition:
/** * @deprecated Use the detailed sets of modified resources instead. */ @Deprecated private int modifiedJSLibs;Apply this to all deprecated fields.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java (1)
55-57: Address the TODO comment for method namingThe TODO comment indicates that this method needs a proper name. Consider renaming it to better reflect its purpose, such as
stageChangesForCommitorprepareGitChanges.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/server/.run/ServerApplication.run.xml(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/dtos/ce/GitStatusCE_DTO.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.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(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java(2 hunks)
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)
962-963: Handle potential null values from getBaseAndBranchedArtifacts
Ensure that the returned artifacts are not null to prevent possible NullPointerException.
990-992:
Check for null branchName in branchedGitMetadata
branchedGitMetadata.getBranchName() may return null. Add a null check before proceeding.
Suggested fix:
final String finalBranchName = branchedGitMetadata.getBranchName();
+ if (!StringUtils.hasText(finalBranchName)) {
+ return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.BRANCH_NAME));
+ }Likely invalid or redundant comment.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java (1)
67-67: LGTM! Clean method signature addition
The new getStatus method follows the established pattern of the interface, using Mono for reactive processing and accepting the standard ArtifactJsonTransformationDTO parameter.
| branchedGitMetadata.setGitAuth(baseGitMetadata.getGitAuth()); | ||
|
|
There was a problem hiding this comment.
Ensure branchedGitMetadata is not null before usage
There is a potential NullPointerException if branchedGitMetadata is null. Add a null check before setting gitAuth.
Suggested fix:
+ if (branchedGitMetadata == null) {
+ return Mono.error(new AppsmithException(AppsmithError.INVALID_GIT_CONFIGURATION, GIT_CONFIG_ERROR));
+ }
branchedGitMetadata.setGitAuth(baseGitMetadata.getGitAuth());Committable suggestion skipped: line range outside the PR's diff.
| @@ -1,6 +1,6 @@ | |||
| <component name="ProjectRunConfigurationManager"> | |||
| <configuration default="false" name="ServerApplication" type="Application" factoryName="Application" nameIsGenerated="true"> | |||
| <option name="ALTERNATIVE_JRE_PATH" value="temurin-17" /> | |||
| <option name="ALTERNATIVE_JRE_PATH" value="/Library/Java/JavaVirtualMachines/openjdk-17.jdk/Contents/Home" /> | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoding absolute JRE paths
Hardcoding the JRE path can lead to issues on different development environments. Consider using a relative path or environment variable.
| @Override | ||
| public Mono<GitStatusDTO> getStatus(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); | ||
|
|
||
| Path repoPath = fsGitHandler.createRepoPath(repoSuffix); | ||
| return fsGitHandler.getStatus(repoPath, refName); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for Git status operations
While the implementation is clean, it should include error handling for potential Git operation failures, similar to other methods in the class.
Consider wrapping the Git status operation with error handling:
@Override
public Mono<GitStatusDTO> getStatus(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);
Path repoPath = fsGitHandler.createRepoPath(repoSuffix);
- return fsGitHandler.getStatus(repoPath, refName);
+ return fsGitHandler.getStatus(repoPath, refName)
+ .onErrorResume(error -> {
+ log.error("Error while fetching git status: {}", error.getMessage());
+ return Mono.error(new AppsmithException(
+ AppsmithError.GIT_ACTION_FAILED,
+ "status",
+ error.getMessage()));
+ });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public Mono<GitStatusDTO> getStatus(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); | |
| Path repoPath = fsGitHandler.createRepoPath(repoSuffix); | |
| return fsGitHandler.getStatus(repoPath, refName); | |
| } | |
| @Override | |
| public Mono<GitStatusDTO> getStatus(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); | |
| Path repoPath = fsGitHandler.createRepoPath(repoSuffix); | |
| return fsGitHandler.getStatus(repoPath, refName) | |
| .onErrorResume(error -> { | |
| log.error("Error while fetching git status: {}", error.getMessage()); | |
| return Mono.error(new AppsmithException( | |
| AppsmithError.GIT_ACTION_FAILED, | |
| "status", | |
| error.getMessage())); | |
| }); | |
| } |
Failed server tests
|
github-actions
bot
added
Git Product
sondermanish
added
ok-to-test
Description
Fixes #37441
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
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12367777643
Commit: 73a48ae
Cypress dashboard.
Tags: @tag.Git
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Sanity/Datasources/RestApiOAuth2Validation_spec.ts
List of identified flaky tests.Tue, 17 Dec 2024 07:58:26 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation