chore: Dummy to run CI on external contributor's PR : add support for Google Cloud Storage in S3 plugin#34075
chore: Dummy to run CI on external contributor's PR : add support for Google Cloud Storage in S3 plugin#34075NilanshBansal wants to merge 11 commits intoreleasefrom
Conversation
…b.com:AnnaHariprasad5123/appsmith into chore/issue-6877/dummy-external-33938-add-gcs
WalkthroughThe recent updates to the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
the
skip-changelog
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9414525091. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.java (2 hunks)
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/constants/S3PluginConstants.java (2 hunks)
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/exceptions/S3ErrorMessages.java (2 hunks)
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/utils/DatasourceUtils.java (6 hunks)
- app/server/appsmith-plugins/amazons3Plugin/src/main/resources/form.json (2 hunks)
- app/server/appsmith-plugins/amazons3Plugin/src/test/java/com/external/plugins/AmazonS3PluginTest.java (2 hunks)
Additional comments not posted (14)
app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/constants/S3PluginConstants.java (3)
8-8: LGTM! EnsureDEFAULT_BUCKET_PROPERTY_INDEXcorrectly corresponds to the actual index used in configurations.
17-17: LGTM! EnsureGOOGLE_CLOUD_SERVICE_PROVIDERis consistently used across the plugin.
18-18: LGTM! EnsureAUTOis used appropriately for automatic region settings in Google Cloud Storage configurations.app/server/appsmith-plugins/amazons3Plugin/src/main/resources/form.json (2)
46-49: LGTM! The addition of "Google Cloud Storage" to the service provider dropdown enhances user options.
115-125: LGTM! The "Default Bucket" configuration is well-integrated with conditional visibility based on the selected service provider.app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/exceptions/S3ErrorMessages.java (2)
21-22: LGTM! The error message for non-existent buckets in Google Cloud Storage is clear and informative.
133-136: LGTM! The error message for missing default bucket parameters is clear and guides the user on what needs to be corrected.app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/utils/DatasourceUtils.java (3)
23-23: LGTM! The added constants are necessary for the new Google Cloud Storage functionality.
71-71: LGTM! The addition ofGOOGLE_CLOUD_STORAGEto theS3ServiceProviderenum allows for proper handling of this service provider.
173-174: LGTM! Setting the region toAUTOfor Google Cloud Storage ingetS3ClientBuilderaligns with the intended functionality.app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.java (2)
106-109: Add constants for Google Cloud Storage support.This addition of constants is necessary for the integration of Google Cloud Storage, ensuring that the plugin can distinguish between different service providers and handle them appropriately.
1017-1043: Implement a separate method for testing Google Cloud Storage.This method correctly handles the specific requirements for testing Google Cloud Storage, such as checking for the existence of a default bucket. It's a good practice to isolate provider-specific logic like this.
app/server/appsmith-plugins/amazons3Plugin/src/test/java/com/external/plugins/AmazonS3PluginTest.java (2)
206-226: Ensure proper validation of the default bucket for Google Cloud Storage.This test method correctly checks for the absence of the default bucket property when using Google Cloud Storage, which aligns with the new requirements. Good use of assertions to validate the expected behavior.
228-250: Validate datasource configuration for Google Cloud Storage with a default bucket.This test method effectively validates that no errors are thrown when the default bucket is provided for Google Cloud Storage. The use of multiple assertions to check that no error messages related to missing parameters are present is commendable.
...erver/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.java
Show resolved
Hide resolved
...erver/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.java
Show resolved
Hide resolved
|
Deploy-Preview-URL: https://ce-34075.dp.appsmith.com |
…b.com:AnnaHariprasad5123/appsmith into chore/issue-6877/dummy-external-33938-add-gcs
NilanshBansal
added
ok-to-test
|
/build-deploy-preview skip-tests=true recreate=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9465398248. |
|
Deploy-Preview-URL: https://ce-34075.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.java (5 hunks)
Additional comments not posted (4)
app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.java (4)
106-109: Constants for Google Cloud Storage have been added correctly.These constants are essential for distinguishing between different service providers and handling specific cases for Google Cloud Storage.
990-994: Separate testing logic for Google Cloud Storage.Splitting the testing logic into a separate method (
testGoogleCloudStorage) enhances modularity and maintainability, especially as different storage providers may require different handling.
964-974: Validate the default bucket for Google Cloud Storage.- if (StringUtils.isNullOrEmpty(defaultBucket)) { + if (properties == null || StringUtils.isNullOrEmpty(defaultBucket)) { invalids.add(S3ErrorMessages.DS_MANDATORY_PARAMETER_DEFAULT_BUCKET_MISSING_ERROR_MSG); }Ensure that the properties list is not null before accessing it to prevent potential
NullPointerException.Likely invalid or redundant comment.
774-778: Ensure correct handling of service provider index.Verification successful
The handling of
S3_SERVICE_PROVIDER_PROPERTY_INDEXappears to be consistent and includes necessary null checks and validations to prevent potential issues.
AmazonS3Plugin.javaandDatasourceUtils.javaboth import and useS3_SERVICE_PROVIDER_PROPERTY_INDEX.- There are multiple instances where null checks are performed before accessing the property using this index.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the service provider index is correctly used throughout the codebase. # Test: Search for the usage of `S3_SERVICE_PROVIDER_PROPERTY_INDEX`. Expect: Consistent usage across the codebase. rg --type java $'S3_SERVICE_PROVIDER_PROPERTY_INDEX'Length of output: 2897
...erver/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.java
Show resolved
Hide resolved
|
Closing this PR as original contribution from the user has been merged. |
2 participants
Description
The original PR is #33938
CI cannot be run on PR which is from a fork other than appsmith. Also, dp and further testing will be done on this PR.
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.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9465389106
Commit: c9639a4
Cypress dashboard url: Click here!
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests