-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
fix: added fix for disabling the autocommit feature without feature flag #34020
Conversation
WalkthroughThe changes introduce a new feature flag annotation Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Client
participant Server
participant FeatureFlagService
Client->>Server: Call isAutoCommitRequired()
Server->>FeatureFlagService: Check release_git_autocommit_feature_enabled
FeatureFlagService-->>Server: Return feature flag status
alt Feature Flag Enabled
Server-->>Client: Perform AutoCommit
else Feature Flag Disabled
Server-->>Client: Skip AutoCommit
end
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: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperImpl.java (1 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/ApplicationPageServiceAutoCommitTest.java (4 hunks)
Files skipped from review due to trivial changes (1)
- app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/ApplicationPageServiceAutoCommitTest.java
Additional comments not posted (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperImpl.java (2)
94-94
: Ensure the@FeatureFlagged
annotation is correctly implemented.
94-94
: Verify the integration of the new method signature across the codebase.Verification successful
The search results show multiple occurrences of the
isAutoCommitRequired
method across various files, including test files and implementation files. The method appears to be used consistently with three parameters, which aligns with the new method signature.
Test Files:
app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/ApplicationPageServiceAutoCommitTest.java
app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperTest.java
Implementation Files:
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java
app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelper.java
app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperImpl.java
app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperFallbackImpl.java
The method is used with the new signature (three parameters) in all instances found. Therefore, the integration of the new method signature appears to be consistent across the codebase.
- : The new method signature
isAutoCommitRequired
is correctly integrated across the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct integration of the new method signature `isAutoCommitRequired` across the codebase. # Test: Search for the method usage. Expect: Only occurrences of the new signature. ast-grep --lang java --pattern $'class $_ { $$$ isAutoCommitRequired($_, $_, $_) { $$$ } $$$ }'Length of output: 96
Script:
#!/bin/bash # Description: Search for all occurrences of the `isAutoCommitRequired` method in the codebase. # Search for the method definition and usage across the codebase. rg 'isAutoCommitRequired' -t javaLength of output: 3758
…lag (#34020) ## Description - Added @featureFlagged annotation `(release_git_autocommit_enabled)` to the interface method `isAutoCommitRequired` for `AutocommitEligibilityHelper` to disable the autocommit as a feature Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!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 --> > [!IMPORTANT] > 🟣 🟣 🟣 Your tests are running. > Tests running at: <https://github.com/appsmithorg/appsmith/actions/runs/9398022580> > Commit: f25b9ef > Workflow: `PR Automation test suite` > Tags: `@tag.Git` <!-- 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** - Introduced a feature flag for auto-commit functionality in the app. - **Tests** - Disabled specific auto-commit test methods. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…lag (#34020) ## Description - Added @featureFlagged annotation `(release_git_autocommit_enabled)` to the interface method `isAutoCommitRequired` for `AutocommitEligibilityHelper` to disable the autocommit as a feature Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!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 --> > [!IMPORTANT] > 🟣 🟣 🟣 Your tests are running. > Tests running at: <https://github.com/appsmithorg/appsmith/actions/runs/9398022580> > Commit: f25b9ef > Workflow: `PR Automation test suite` > Tags: `@tag.Git` <!-- 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** - Introduced a feature flag for auto-commit functionality in the app. - **Tests** - Disabled specific auto-commit test methods. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
(release_git_autocommit_enabled)
to the interface methodisAutoCommitRequired
forAutocommitEligibilityHelper
to disable the autocommit as a featureFixes #
Issue Number
or
Fixes
Issue URL
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/9398022580
Commit: f25b9ef
Cypress dashboard url: Click here!
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Tests