chore: git changes for new contract#39376
Conversation
WalkthroughThis pull request introduces significant modifications across several files related to Git operations. The changes include integrating a new dependency ( Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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: 0
🧹 Nitpick comments (4)
app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java (3)
88-88: Visibility check.
DeclaringfsGitHandlerasprotectedsuggests its usage in subclasses. Ensure this level of visibility is truly needed.
271-303: New whitelisting methods: getWhitelistedPaths and isWhiteListedPath.
These methods provide a clear approach for filtering paths. Consider:
- Documenting how this is updated if more whitelisted paths arise in future.
- Validating folder-depth edge cases.
305-305: Integrating whitelisted logic in getExistingFilesInRepo.
InvokinggetWhitelistedPaths()at each call may be fine now, but if performance becomes a concern, cache the result.app/server/appsmith-git/src/test/java/com/appsmith/git/helpers/FileUtilsImplTest.java (1)
35-36: Added FSGitHandler test field.
Ensure it’s properly mocked or initialized to cover all new logic inFileUtilsImpl.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java(6 hunks)app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsImpl.java(2 hunks)app/server/appsmith-git/src/test/java/com/appsmith/git/helpers/FileUtilsImplTest.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/ApplicationCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java(3 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-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (15)
app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java (6)
9-9: Good addition of the FSGitHandler import.
This aligns with the refactoring to delegate Git resets toFSGitHandler.
79-79: Ensure consistency of path delimiters.
UsingDELIMITER_PATHfromCommonConstantsCEis fine on Unix-like systems, but verify cross-platform compatibility (e.g., Windows) to avoid path resolution issues.
107-108: Constructor dependency injection update.
InjectingFSGitHandlerimproves modularity for file-system-related Git operations. No concerns here.
113-113: Field assignment clarity.
Assigningthis.fsGitHandler = fsGitHandler;is straightforward. No issues.
252-269: Switching to fsGitHandler for resetToLastCommit.
Migrating the responsibility fromgitExecutortofsGitHandleris consistent with the new abstraction. Confirm that theresetToLastCommitlogic remains equivalent and that any error handling needed is in place.
309-312: Filtering non-whitelisted files.
Appending theisWhiteListedPathcheck ensures only allowed paths remain. Looks correct for ignoring.git/files.app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsImpl.java (3)
5-5: Importing FSGitHandler.
No issues. This facilitates consistent usage of the new handler throughout.
25-25: Adding FSGitHandler to the constructor signature.
Ensures this class stays in sync with the refactored superclass constructor.
30-30: Passing fsGitHandler to super.
Correctly forwards the parameter for the new file-system-based Git operations.app/server/appsmith-git/src/test/java/com/appsmith/git/helpers/FileUtilsImplTest.java (2)
3-3: New import of FSGitHandler.
Indicates test coverage forfsGitHandlerusage.
48-54: Updated constructor call in setUp.
PassingfsGitHandleris necessary for the new design. Confirm the mocking behavior if needed.app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/ApplicationCE.java (1)
371-374: LGTM! Making artifact type publicly visible is appropriate.The change from
@JsonView(Views.Internal.class)to@JsonView(Views.Public.class)forgetArtifactType()is appropriate since it's a read-only value that identifies the type of artifact.app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (2)
218-226: LGTM! Path handling improvements.The changes improve clarity by:
- Renaming
temporaryStoragetotemporaryArtifactPath- Simplifying path construction to use only workspaceId and placeholder
- Using the renamed
moveRepositoryFromTemporaryStoragemethod
242-264: LGTM! Improved error handling structure.The error handling refactoring enhances code clarity and maintainability by:
- Separating concerns into distinct Mono variables
- Using Mono.zip to properly combine operations
- Providing clear error messages based on exception types
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (1)
789-816: LGTM! Improved method naming and error handling.The changes enhance code clarity and maintainability through:
- More descriptive method name
moveRepositoryFromTemporaryStorage- Better parameter names that reflect their purpose
- Enhanced error logging with detailed path information
Failed server tests
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/ExchangeJsonConversionTests.java (1)
132-144: Consider using temporary directory for test files.The test creates and deletes directories in a hardcoded path. Consider using
@TempDirorFiles.createTempDirectory()for better test isolation and cleanup.Example refactor:
-Files.createDirectories(Path.of("./container-volumes/git-storage/test123")); +Path tempDir = Files.createTempDirectory("git-test"); +Path testPath = tempDir.resolve("test123"); +Files.createDirectories(testPath); Mono<Path> responseMono = - commonGitFileUtils.saveArtifactToLocalRepoNew(Path.of("test123"), originalArtifactJson, "irrelevant"); + commonGitFileUtils.saveArtifactToLocalRepoNew(testPath, originalArtifactJson, "irrelevant"); -FileUtils.deleteDirectory( - Path.of("./container-volumes/git-storage/test123").toFile()); +FileUtils.deleteDirectory(tempDir.toFile());
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/ExchangeJsonConversionTests.java(3 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-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (3)
app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/ExchangeJsonConversionTests.java (3)
3-3: LGTM!Clean import of the new FSGitHandler dependency.
55-55: LGTM!Correct replacement of GitExecutor with FSGitHandler in the SpyBean annotation.
130-130: LGTM!Mock behavior correctly updated to use FSGitHandler while maintaining the same return type and parameter structure.
## Description - bug solutions and error handling ## 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/13436821247> > Commit: 4477846 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13436821247&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git` > Spec: > <hr>Thu, 20 Feb 2025 14:40:06 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** - Artifact type information is now publicly visible in application responses. - **Refactor** - Enhanced repository and file operations with improved path validation, error handling, and updated dependency integration. - **Tests** - Updated test configurations to align with the revised dependency setup and file handling enhancements. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
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/13436821247
Commit: 4477846
Cypress dashboard.
Tags:
@tag.GitSpec:
Thu, 20 Feb 2025 14:40:06 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit