Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request comprehensively removes tunnel-based SSH connectivity and Aliyun RDS cloud database integration from the codebase. It eliminates three model classes ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api_instances/views.py (1)
150-150:⚠️ Potential issue | 🟡 MinorStale wording in OpenAPI description.
After dropping tunnel/Aliyun RDS filters, "legacy inventory filters" is misleading — this endpoint's filters (search/type/db_type/tags/ordering) are the current ones, not legacy. Consider rewording to avoid confusion in the generated schema/docs.
✏️ Proposed rewording
- description="List all instances with pagination, search, and legacy inventory filters.", + description="List all instances with pagination, search, and inventory filters.",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api_instances/views.py` at line 150, Update the OpenAPI description string used in the instances list endpoint in api_instances/views.py (the description argument shown) to remove the phrase "legacy inventory filters" and clearly enumerate the current filters; replace with a concise statement such as "List instances with pagination, search and filters (type, db_type, tags, ordering)." Ensure this new text is set on the same description parameter for the view/function so the generated schema/docs reflect the current filters.sql/admin.py (1)
451-454:⚠️ Potential issue | 🟡 MinorBlack formatting failing — add blank line between top-level class blocks.
Per pipeline failure, Black requires two blank lines before the
# Login audit log/@admin.register(AuditEntry)top-level block followingArchiveConfigAdmin.🎨 Proposed fix
"user_name", "user_display", ) + + # Login audit log `@admin.register`(AuditEntry) class AuditEntryAdmin(admin.ModelAdmin):As per coding guidelines: "For Python code, use Black for formatting and ensure
black --check .passes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/admin.py` around lines 451 - 454, Add the required two blank lines between the end of the ArchiveConfigAdmin class block and the next top-level block so Black formatting passes; specifically insert the blank lines before the comment "# Login audit log" / decorator "@admin.register(AuditEntry)" that registers the AuditEntry admin to ensure there are two blank lines separating ArchiveConfigAdmin and the AuditEntry registration.
🧹 Nitpick comments (2)
sql/engines/__init__.py (2)
33-44: Optional: simplifyremote_instance_connnow that tunneling is gone.With SSH tunnel resolution removed, storing
remote_host/remote_port/remote_user/remote_passwordonselfis an unused side effect — callers (e.g.sql/engines/goinception.py:99) only use the returned tuple. Consider dropping the instance-attribute assignments (or deprecating the method in favor ofinstance.get_username_password()directly at call sites).♻️ Proposed simplification
def remote_instance_conn(self, instance=None): user, password = instance.get_username_password() - self.remote_host = instance.host - self.remote_port = instance.port - self.remote_user = user - self.remote_password = password - return ( - self.remote_host, - self.remote_port, - self.remote_user, - self.remote_password, - ) + return instance.host, instance.port, user, password🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/engines/__init__.py` around lines 33 - 44, The method remote_instance_conn currently assigns remote_host/remote_port/remote_user/remote_password on self even though callers only use its return tuple; remove those instance-attribute side effects and make remote_instance_conn simply return (instance.host, instance.port, user, password) after calling instance.get_username_password(), or consider removing remote_instance_conn and update call sites (e.g., places that call remote_instance_conn) to use instance.get_username_password() plus instance.host/instance.port directly.
30-31: Remove the now-empty__del__method.A
__del__that only doespassis not a no-op: defining__del__historically prevented CPython's cycle collector from collecting cycles containing the object (see PEP 442; much improved since 3.4 but still a smell and can still interact poorly with weakrefs/finalizers). Since all SSH-cleanup logic is gone, drop the method entirely.♻️ Proposed cleanup
def __init__(self, instance: Instance = None): self.conn = None self.thread_id = None if instance: self.instance = instance # type: Instance self.instance_name = instance.instance_name self.host = instance.host self.port = int(instance.port) self.user, self.password = self.instance.get_username_password() self.db_name = instance.db_name self.mode = instance.mode - def __del__(self): - pass - def remote_instance_conn(self, instance=None):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/engines/__init__.py` around lines 30 - 31, Remove the empty destructor by deleting the __del__ method definition from the class (i.e., drop the def __del__(self): pass block) so the class no longer defines a no-op finalizer; simply remove that method and leave the class without a __del__ implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sql/form.py`:
- Around line 12-14: The file fails Black formatting because there are not two
blank lines between top-level imports and the class definition; add the required
blank line separation before the class InstanceForm so the class definition
(class InstanceForm(ModelForm):) is preceded by two blank lines after the import
block (from django.forms import ModelForm and from sql.models import Instance)
to satisfy black --check ..
In `@sql/migrations/0013_remove_aliyunrdsconfig_ak_and_more.py`:
- Around line 1-34: The migration file's string quotes are single quotes and
fail the project's Black formatting check; open the Migration class in this
migration and normalize all string literals (e.g., entries in dependencies and
model names in operations like RemoveField/DeleteModel for 'aliyunrdsconfig',
'instance', 'tunnel', 'CloudAccessKey', 'AliyunRdsConfig', 'Tunnel') to use
Black's preferred formatting by running Black on the file (black
sql/migrations/0013_remove_aliyunrdsconfig_ak_and_more.py) or on the repo and
then re-commit once black --check . passes.
In `@sql/slowlog.py`:
- Around line 48-50: Collapse the multiline datetime arithmetic so the strptime
call and timedelta addition are on a single line in both functions
slowquery_review and slowquery_review_history: replace the current multi-line
expression that spans strptime(...) + datetime.timedelta(days=1) with a
single-line expression (end_time = datetime.datetime.strptime(end_time,
"%Y-%m-%d") + datetime.timedelta(days=1)) in each function so Black formatting
passes.
---
Outside diff comments:
In `@api_instances/views.py`:
- Line 150: Update the OpenAPI description string used in the instances list
endpoint in api_instances/views.py (the description argument shown) to remove
the phrase "legacy inventory filters" and clearly enumerate the current filters;
replace with a concise statement such as "List instances with pagination, search
and filters (type, db_type, tags, ordering)." Ensure this new text is set on the
same description parameter for the view/function so the generated schema/docs
reflect the current filters.
In `@sql/admin.py`:
- Around line 451-454: Add the required two blank lines between the end of the
ArchiveConfigAdmin class block and the next top-level block so Black formatting
passes; specifically insert the blank lines before the comment "# Login audit
log" / decorator "@admin.register(AuditEntry)" that registers the AuditEntry
admin to ensure there are two blank lines separating ArchiveConfigAdmin and the
AuditEntry registration.
---
Nitpick comments:
In `@sql/engines/__init__.py`:
- Around line 33-44: The method remote_instance_conn currently assigns
remote_host/remote_port/remote_user/remote_password on self even though callers
only use its return tuple; remove those instance-attribute side effects and make
remote_instance_conn simply return (instance.host, instance.port, user,
password) after calling instance.get_username_password(), or consider removing
remote_instance_conn and update call sites (e.g., places that call
remote_instance_conn) to use instance.get_username_password() plus
instance.host/instance.port directly.
- Around line 30-31: Remove the empty destructor by deleting the __del__ method
definition from the class (i.e., drop the def __del__(self): pass block) so the
class no longer defines a no-op finalizer; simply remove that method and leave
the class without a __del__ implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4163dca1-fa47-4436-9495-8981ff7030b3
📒 Files selected for processing (32)
api_core/legacy_tests.pyapi_instances/serializers.pyapi_instances/tests.pyapi_instances/urls.pyapi_instances/views.pycommon/utils/aliyun_sdk.pyfrontend/src/features/inventory/pages/InventoryEditorPage.vuefrontend/src/lib/api.tsrequirements.txtsql/admin.pysql/engines/__init__.pysql/engines/cloud/aliyun_rds.pysql/engines/goinception.pysql/engines/tests.pysql/form.pysql/management/commands/reencrypt_sensitive_fields.pysql/migrations/0013_remove_aliyunrdsconfig_ak_and_more.pysql/models.pysql/slowlog.pysql/test_reencrypt_sensitive_fields.pysql/tests.pysql/utils/ssh_tunnel.pysrc/init_sql/goinception_param_template.sqlsrc/init_sql/v1.0_init.sqlsrc/init_sql/v1.2.0.sqlsrc/init_sql/v1.3.7.sqlsrc/init_sql/v1.4.5.sqlsrc/init_sql/v1.5.0.sqlsrc/init_sql/v1.5.3_comment.sqlsrc/init_sql/v1.7.11.sqlsrc/init_sql/v1.7.12.sqlsrc/init_sql/v1.8.3.sql
💤 Files with no reviewable changes (17)
- requirements.txt
- api_instances/tests.py
- src/init_sql/goinception_param_template.sql
- src/init_sql/v1.0_init.sql
- src/init_sql/v1.5.3_comment.sql
- sql/engines/goinception.py
- api_instances/urls.py
- api_core/legacy_tests.py
- sql/engines/tests.py
- sql/models.py
- src/init_sql/v1.4.5.sql
- sql/engines/cloud/aliyun_rds.py
- sql/management/commands/reencrypt_sensitive_fields.py
- common/utils/aliyun_sdk.py
- frontend/src/lib/api.ts
- sql/utils/ssh_tunnel.py
- api_instances/serializers.py
Comment on lines
+12
to
14
| from django.forms import ModelForm | ||
| from sql.models import Instance | ||
| class InstanceForm(ModelForm): |
Contributor
There was a problem hiding this comment.
Black formatting failing — missing blank lines before class.
Per pipeline failure, Black requires two blank lines between the imports and the InstanceForm class definition.
🎨 Proposed fix
from django.forms import ModelForm
from sql.models import Instance
+
+
class InstanceForm(ModelForm):
class Media:As per coding guidelines: "For Python code, use Black for formatting and ensure black --check . passes".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sql/form.py` around lines 12 - 14, The file fails Black formatting because
there are not two blank lines between top-level imports and the class
definition; add the required blank line separation before the class InstanceForm
so the class definition (class InstanceForm(ModelForm):) is preceded by two
blank lines after the import block (from django.forms import ModelForm and from
sql.models import Instance) to satisfy black --check ..
Comment on lines
+1
to
+34
| # Generated by Django 6.0.3 on 2026-04-20 21:30 | ||
|
|
||
| from django.db import migrations | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ('sql', '0012_archiveconfig_consecutive_failures_and_more'), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.RemoveField( | ||
| model_name='aliyunrdsconfig', | ||
| name='ak', | ||
| ), | ||
| migrations.RemoveField( | ||
| model_name='aliyunrdsconfig', | ||
| name='instance', | ||
| ), | ||
| migrations.RemoveField( | ||
| model_name='instance', | ||
| name='tunnel', | ||
| ), | ||
| migrations.DeleteModel( | ||
| name='CloudAccessKey', | ||
| ), | ||
| migrations.DeleteModel( | ||
| name='AliyunRdsConfig', | ||
| ), | ||
| migrations.DeleteModel( | ||
| name='Tunnel', | ||
| ), | ||
| ] |
Contributor
There was a problem hiding this comment.
Black formatting check failing — normalize quotes.
Per pipeline failure, Black wants double quotes throughout this migration. makemigrations emits single quotes, but the repo enforces Black, so either pipe the output through black or run black sql/migrations/0013_remove_aliyunrdsconfig_ak_and_more.py before committing.
🎨 Proposed Black-compliant version
-# Generated by Django 6.0.3 on 2026-04-20 21:30
-
-from django.db import migrations
-
-
-class Migration(migrations.Migration):
-
- dependencies = [
- ('sql', '0012_archiveconfig_consecutive_failures_and_more'),
- ]
-
- operations = [
- migrations.RemoveField(
- model_name='aliyunrdsconfig',
- name='ak',
- ),
- migrations.RemoveField(
- model_name='aliyunrdsconfig',
- name='instance',
- ),
- migrations.RemoveField(
- model_name='instance',
- name='tunnel',
- ),
- migrations.DeleteModel(
- name='CloudAccessKey',
- ),
- migrations.DeleteModel(
- name='AliyunRdsConfig',
- ),
- migrations.DeleteModel(
- name='Tunnel',
- ),
- ]
+# Generated by Django 6.0.3 on 2026-04-20 21:30
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+ dependencies = [
+ ("sql", "0012_archiveconfig_consecutive_failures_and_more"),
+ ]
+
+ operations = [
+ migrations.RemoveField(
+ model_name="aliyunrdsconfig",
+ name="ak",
+ ),
+ migrations.RemoveField(
+ model_name="aliyunrdsconfig",
+ name="instance",
+ ),
+ migrations.RemoveField(
+ model_name="instance",
+ name="tunnel",
+ ),
+ migrations.DeleteModel(
+ name="CloudAccessKey",
+ ),
+ migrations.DeleteModel(
+ name="AliyunRdsConfig",
+ ),
+ migrations.DeleteModel(
+ name="Tunnel",
+ ),
+ ]As per coding guidelines: "For Python code, use Black for formatting and ensure black --check . passes".
Note: The Ruff RUF012 "mutable default value for class attribute" hints on dependencies/operations are false positives — this is the standard Django Migration class shape produced by makemigrations and does not need ClassVar annotations.
📝 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.
Suggested change
| # Generated by Django 6.0.3 on 2026-04-20 21:30 | |
| from django.db import migrations | |
| class Migration(migrations.Migration): | |
| dependencies = [ | |
| ('sql', '0012_archiveconfig_consecutive_failures_and_more'), | |
| ] | |
| operations = [ | |
| migrations.RemoveField( | |
| model_name='aliyunrdsconfig', | |
| name='ak', | |
| ), | |
| migrations.RemoveField( | |
| model_name='aliyunrdsconfig', | |
| name='instance', | |
| ), | |
| migrations.RemoveField( | |
| model_name='instance', | |
| name='tunnel', | |
| ), | |
| migrations.DeleteModel( | |
| name='CloudAccessKey', | |
| ), | |
| migrations.DeleteModel( | |
| name='AliyunRdsConfig', | |
| ), | |
| migrations.DeleteModel( | |
| name='Tunnel', | |
| ), | |
| ] | |
| # Generated by Django 6.0.3 on 2026-04-20 21:30 | |
| from django.db import migrations | |
| class Migration(migrations.Migration): | |
| dependencies = [ | |
| ("sql", "0012_archiveconfig_consecutive_failures_and_more"), | |
| ] | |
| operations = [ | |
| migrations.RemoveField( | |
| model_name="aliyunrdsconfig", | |
| name="ak", | |
| ), | |
| migrations.RemoveField( | |
| model_name="aliyunrdsconfig", | |
| name="instance", | |
| ), | |
| migrations.RemoveField( | |
| model_name="instance", | |
| name="tunnel", | |
| ), | |
| migrations.DeleteModel( | |
| name="CloudAccessKey", | |
| ), | |
| migrations.DeleteModel( | |
| name="AliyunRdsConfig", | |
| ), | |
| migrations.DeleteModel( | |
| name="Tunnel", | |
| ), | |
| ] |
🧰 Tools
🪛 GitHub Actions: Lint
[error] 1-1: Black formatting check failed. Would reformat this file (quote normalization and formatting changes detected).
🪛 Ruff (0.15.10)
[warning] 8-10: Mutable default value for class attribute
(RUF012)
[warning] 12-34: Mutable default value for class attribute
(RUF012)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sql/migrations/0013_remove_aliyunrdsconfig_ak_and_more.py` around lines 1 -
34, The migration file's string quotes are single quotes and fail the project's
Black formatting check; open the Migration class in this migration and normalize
all string literals (e.g., entries in dependencies and model names in operations
like RemoveField/DeleteModel for 'aliyunrdsconfig', 'instance', 'tunnel',
'CloudAccessKey', 'AliyunRdsConfig', 'Tunnel') to use Black's preferred
formatting by running Black on the file (black
sql/migrations/0013_remove_aliyunrdsconfig_ak_and_more.py) or on the repo and
then re-commit once black --check . passes.
Comment on lines
+48
to
+50
| end_time = datetime.datetime.strptime( | ||
| end_time, "%Y-%m-%d" | ||
| ) + datetime.timedelta(days=1) |
Contributor
There was a problem hiding this comment.
Black formatting failing — collapse datetime.timedelta onto one line.
Per pipeline failure, Black wants the strptime(...) + datetime.timedelta(days=1) expressions on a single line (they fit well under the line limit).
🎨 Proposed fix
- end_time = datetime.datetime.strptime(
- end_time, "%Y-%m-%d"
- ) + datetime.timedelta(days=1)
+ end_time = datetime.datetime.strptime(end_time, "%Y-%m-%d") + datetime.timedelta(
+ days=1
+ )Apply the same change to both slowquery_review (lines 48-50) and slowquery_review_history (lines 139-141).
As per coding guidelines: "For Python code, use Black for formatting and ensure black --check . passes".
Also applies to: 139-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sql/slowlog.py` around lines 48 - 50, Collapse the multiline datetime
arithmetic so the strptime call and timedelta addition are on a single line in
both functions slowquery_review and slowquery_review_history: replace the
current multi-line expression that spans strptime(...) +
datetime.timedelta(days=1) with a single-line expression (end_time =
datetime.datetime.strptime(end_time, "%Y-%m-%d") + datetime.timedelta(days=1))
in each function so Black formatting passes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Summary by CodeRabbit
Release Notes