-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
feat: add support for Google Cloud Storage in S3 plugin #33938
feat: add support for Google Cloud Storage in S3 plugin #33938
Conversation
WalkthroughThe changes introduce support for Google Cloud Storage in the S3 plugin by adding necessary constants, updating logic to handle Google Cloud Storage as a service provider, and adding new test cases to validate these configurations. The changes span across multiple files, including Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Appsmith
participant GoogleCloudStorage
participant AmazonS3
User->>Appsmith: Configure Google Cloud Storage
Appsmith->>GoogleCloudStorage: Validate Configuration
GoogleCloudStorage-->>Appsmith: Return Validation Result
Appsmith-->>User: Show Validation Result
User->>Appsmith: Configure Amazon S3
Appsmith->>AmazonS3: Validate Configuration
AmazonS3-->>Appsmith: Return Validation Result
Appsmith-->>User: Show Validation Result
Assessment against linked issues
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 (
|
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
Outside diff range and nitpick comments (1)
app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.java (1)
103-103
: Review the static imports for clarity and necessity.The static imports from
S3PluginConstants
are extensive. Consider importing only the necessary constants to improve code readability and maintainability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Commits
Files that changed from the base of the PR and between 3c38ca9 and dd2158456c5eccb270f4c5e9bcda6837b5ca68f1.
Files selected for processing (6)
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.java (4 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 (3 hunks)
Additional comments not posted (11)
app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/constants/S3PluginConstants.java (1)
Line range hint
8-19
: The new constants for Google Cloud Storage are correctly defined and align with the PR's objectives to support GCS in the S3 plugin.app/server/appsmith-plugins/amazons3Plugin/src/main/resources/form.json (2)
46-49
: The addition of "Google Cloud Storage" to the service provider dropdown is correctly implemented.
115-125
: The new "Default Bucket" field is correctly added and configured to be visible only when Google Cloud Storage is selected. This aligns with the PR's objectives to handle GCS-specific configurations.app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/exceptions/S3ErrorMessages.java (2)
21-23
: The new error message for non-existent buckets in Google Cloud Storage is appropriately detailed and will enhance error reporting.
133-137
: The error message for missing "Default Bucket" parameter is clear and necessary for ensuring that all required parameters are provided for Google Cloud Storage.app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/utils/DatasourceUtils.java (2)
23-23
: The addition ofGOOGLE_CLOUD_STORAGE
to theS3ServiceProvider
enum and the import adjustments are correctly implemented to support the new functionality.Also applies to: 68-68
157-163
: The modifications in thegetS3ClientBuilder
method to handle Google Cloud Storage are correctly implemented. The use of theAUTO
region setting aligns with GCS's requirements.app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.java (2)
1016-1042
: Review the methodtestGoogleCloudStorage
.The method
testGoogleCloudStorage
is well-implemented with clear error handling and resource management. It correctly handles different scenarios such as missing default bucket and potential S3 exceptions.
952-962
: Validate the handling of theDEFAULT_BUCKET_PROPERTY_INDEX
.Ensure that the
DEFAULT_BUCKET_PROPERTY_INDEX
is being handled correctly across the codebase, especially in contexts where it might affect the configuration or operation of the plugin.Verification successful
The handling of
DEFAULT_BUCKET_PROPERTY_INDEX
is consistent and correct in the codebase. The property is retrieved and validated appropriately in both occurrences withinAmazonS3Plugin.java
.
AmazonS3Plugin.java
:
- Lines 952-962: Retrieves and validates
defaultBucket
.testGoogleCloudStorage
method: Retrieves and validatesdefaultBucket
.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the handling of the DEFAULT_BUCKET_PROPERTY_INDEX in other parts of the codebase. # Test: Search for the usage of DEFAULT_BUCKET_PROPERTY_INDEX. Expect: Proper handling in all occurrences. rg --type java 'DEFAULT_BUCKET_PROPERTY_INDEX'Length of output: 589
Script:
#!/bin/bash # Extract lines around the second occurrence of DEFAULT_BUCKET_PROPERTY_INDEX in AmazonS3Plugin.java rg --type java --context 10 'DEFAULT_BUCKET_PROPERTY_INDEX' app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.javaLength of output: 2324
app/server/appsmith-plugins/amazons3Plugin/src/test/java/com/external/plugins/AmazonS3PluginTest.java (2)
202-222
: The test methodtestValidateDatasourceWithMissingRegionWithNonAmazonProvider
effectively checks for the mandatory default bucket parameter when using a non-Amazon provider. This aligns with the PR objectives to handle Google Cloud Storage configurations differently.
73-73
: Ensure that the wildcard import fromS3PluginConstants
is necessary and used efficiently.Verification successful
Ensure that the wildcard import from
S3PluginConstants
is necessary and used efficiently.
The wildcard import from
S3PluginConstants
is necessary and used efficiently, as evidenced by the significant number of constant usages in the test file.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all constants from S3PluginConstants are used in the test file. # Test: Search for the usage of constants from S3PluginConstants. Expect: Multiple usages. rg --type java --count 'S3PluginConstants\.\w+' app/server/appsmith-plugins/amazons3Plugin/src/test/java/com/external/plugins/AmazonS3PluginTest.javaLength of output: 151
Script:
#!/bin/bash # Description: Verify that all constants from S3PluginConstants are used in the test file. # Test: Search for the usage of constants from S3PluginConstants without the class prefix. Expect: Multiple usages. rg --type java --count '\b(CONSTANT_NAME_1|CONSTANT_NAME_2|CONSTANT_NAME_3|...)\b' app/server/appsmith-plugins/amazons3Plugin/src/test/java/com/external/plugins/AmazonS3PluginTest.javaLength of output: 190
...erver/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.java
Outdated
Show resolved
Hide resolved
dd21584
to
cdfc64f
Compare
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Commits
Files that changed from the base of the PR and between dd2158456c5eccb270f4c5e9bcda6837b5ca68f1 and cdfc64f.
Files selected for processing (6)
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.java (3 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 (3 hunks)
Files skipped from review as they are similar to previous changes (6)
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.java
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/constants/S3PluginConstants.java
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/exceptions/S3ErrorMessages.java
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/utils/DatasourceUtils.java
- app/server/appsmith-plugins/amazons3Plugin/src/main/resources/form.json
- app/server/appsmith-plugins/amazons3Plugin/src/test/java/com/external/plugins/AmazonS3PluginTest.java
Hi @nidhi-nair, Could you review this pr. |
Thanks for the contribution @AnnaHariprasad5123 ! @NilanshBansal will be picking up the review for this PR, I have assigned it to him. |
...erver/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.java
Outdated
Show resolved
Hide resolved
cdfc64f
to
6a3599e
Compare
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Commits
Files that changed from the base of the PR and between cdfc64f and 6a3599e806ba6d8038f614eeab597ed90ca615f9.
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 (3 hunks)
Files skipped from review as they are similar to previous changes (6)
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.java
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/constants/S3PluginConstants.java
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/exceptions/S3ErrorMessages.java
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/utils/DatasourceUtils.java
- app/server/appsmith-plugins/amazons3Plugin/src/main/resources/form.json
- app/server/appsmith-plugins/amazons3Plugin/src/test/java/com/external/plugins/AmazonS3PluginTest.java
@AnnaHariprasad5123 spotless checks are failing on the PR. |
Hi @NilanshBansal, I run it. Everything is passed on my side. This is the screenshot : |
6a3599e
to
4844aed
Compare
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Commits
Files that changed from the base of the PR and between 6a3599e806ba6d8038f614eeab597ed90ca615f9 and 4844aed.
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 (3 hunks)
Files skipped from review as they are similar to previous changes (6)
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.java
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/constants/S3PluginConstants.java
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/exceptions/S3ErrorMessages.java
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/utils/DatasourceUtils.java
- app/server/appsmith-plugins/amazons3Plugin/src/main/resources/form.json
- app/server/appsmith-plugins/amazons3Plugin/src/test/java/com/external/plugins/AmazonS3PluginTest.java
@AnnaHariprasad5123 after running If I run |
HI @NilanshBansal Sorry for this, Could you check again. Is it fine? |
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/utils/DatasourceUtils.java (5 hunks)
- app/server/appsmith-plugins/amazons3Plugin/src/test/java/com/external/plugins/AmazonS3PluginTest.java (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/utils/DatasourceUtils.java
- app/server/appsmith-plugins/amazons3Plugin/src/test/java/com/external/plugins/AmazonS3PluginTest.java
...server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/utils/DatasourceUtils.java
Outdated
Show resolved
Hide resolved
...r/appsmith-plugins/amazons3Plugin/src/test/java/com/external/plugins/AmazonS3PluginTest.java
Outdated
Show resolved
Hide resolved
...r/appsmith-plugins/amazons3Plugin/src/test/java/com/external/plugins/AmazonS3PluginTest.java
Show resolved
Hide resolved
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/utils/DatasourceUtils.java (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/utils/DatasourceUtils.java
@AnnaHariprasad5123 thanks for your contribution 🙏 , the code LGTM.
Further checks, will be tested on this dummy PR. Once the checks pass, we will merge your original PR to appsmith!! |
@AnnaHariprasad5123 the ![]() ![]() |
Hi @NilanshBansal, please review all the issues and let me know. I will make all the necessary changes at once. |
@AnnaHariprasad5123 I have reviewed. Everything else LGTM from your PR perspective. |
Hi @NilanshBansal, Did you check the IAM permissions for the bucket? Have you tried performing the same operation with the AmazonS3 provider? I didn't make any changes to the query code, but it seems that the deleteMultipleFiles method is the only one not working. So, can we merge this PR and raise an issue on this?. I am currently occupied with other work and unable to continue this investigation. Please let me know what's your opinion. Thank you. |
Hey @AnnaHariprasad5123,
|
@NilanshBansal , I believe there is an alternative approach for handling batch deletions in GCS, different from Amazon S3. We could iterate through each file in listOfFiles and perform individual deletions, as single deletions are working. Do you think this is a good approach? If so, I will implement it and push the changes. Please share your opinion. |
@AnnaHariprasad5123 Thanks for this analysis, yes completely agreed with you, handling batch deletions in GCS is different from AWS S3. Saying that, we can take up the implementation of Delete Multiple Files option for GCS as a separate issue. We would love to get your contributions on the same in a separate PR. |
Hi @NilanshBansal, I'm trying to hide the deleteMultipleFiles label in root.json, located in the editor folder. However, the changes I made aren't showing up. Can you help me understand why?
Even when I try to add a new label, it's still not showing up. |
@AnnaHariprasad5123 to view the change you may need to run this command before starting the server to compile the amazonS3Plugin, then only the changes will show up. |
Yes I do it every time. Even though not reflected. If you don't mind. Could you put that code snippet in that file and observe if change is reflected or not. |
Hi @NilanshBansal, Is it working from your side? Please let me know. |
@AnnaHariprasad5123 on debugging, I have found that we do not have the hidden option implemented for DROP_DOWN.options internally. |
@AnnaHariprasad5123 until I am figuring out how to hide an option from the dropdown. |
Hi @NilanshBansal , It works for every S3 provider, Could you check for GCS and amazon S3. Let me know if any changes required. We can create an issue to optimize this operation further if this approach is not ok. This is my opinion. |
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
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 (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.java
...erver/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.java
Outdated
Show resolved
Hide resolved
294d18f
to
baae8b6
Compare
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
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)
Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.java
Hi @NilanshBansal, Is this ok now. Let me know. |
Hi @NilanshBansal Good morning, I've completed the necessary changes. Kindly consider merging my pull request when you get a chance. |
Thank you for your contribution and patience @AnnaHariprasad5123! 🙏 |
Fixes #6877
Screenshots :
Video Demonstration :
Link
Summary by CodeRabbit
New Features
Bug Fixes
Tests