Skip to content

fix: use ModifyRDSDBInstance endpoint for RDS modify commands#254

Merged
kferrone merged 12 commits intomainfrom
fix/rds-modify-endpoint
Apr 24, 2026
Merged

fix: use ModifyRDSDBInstance endpoint for RDS modify commands#254
kferrone merged 12 commits intomainfrom
fix/rds-modify-endpoint

Conversation

@amaechiabuah
Copy link
Copy Markdown
Collaborator

Describe Changes

retention_period, iam_auth, and final_snapshot were using the generic V3 update() method, which does a PUT to the V3 RDS endpoint and calls response.json() on the result. The ModifyRDSDBInstance API returns an empty response body, causing a JSON decode error.

Changed all three commands to use self.client.post(subscriptions/{tenant_id}/ModifyRDSDBInstance, body) directly — matching the existing pattern used by set_monitor_interval.

Link to Issues

https://app.clickup.com/t/8655600/DUPLO-41909

PR Review Checklist

  • Thoroughly reviewed on local machine.
  • Have you added any tests
  • Make sure to note changes in Changelog

@zafarabbas
Copy link
Copy Markdown
Contributor

@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Fix RDS modify commands using correct endpoint

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Replace generic update() method with ModifyRDSDBInstance endpoint
• Fix JSON decode error in retention_period, iam_auth, final_snapshot
• Correct docstring for final_snapshot method
• Update CHANGELOG with fix details
Diagram
flowchart LR
  A["RDS modify methods<br/>retention_period, iam_auth,<br/>final_snapshot"] -->|"Previously: generic<br/>update() method"| B["V3 RDS endpoint<br/>PUT request"]
  B -->|"Empty response body"| C["JSON decode error"]
  A -->|"Now: Direct POST"| D["ModifyRDSDBInstance<br/>endpoint"]
  D -->|"Proper handling"| E["Success"]
Loading

Grey Divider

File Changes

1. src/duplo_resource/rds.py 🐞 Bug fix +7/-4

Replace update method with ModifyRDSDBInstance endpoint

• Replaced self.update() calls with self.client.post() to ModifyRDSDBInstance endpoint in
 three methods
• Extract tenant_id from self.tenant["TenantId"] for endpoint URL construction
• Fixed incorrect docstring in final_snapshot() method (was "Toggle IAM authentication")

src/duplo_resource/rds.py


2. CHANGELOG.md 📝 Documentation +1/-0

Document RDS modify endpoint fix

• Added entry documenting fix for rds retention_period, rds iam_auth, and rds final_snapshot
 JSON decode error
• Clarified that fix uses correct ModifyRDSDBInstance endpoint

CHANGELOG.md


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Apr 14, 2026

Code Review by Qodo

🐞 Bugs (1)   📘 Rule violations (1)   📎 Requirement gaps (0)
🐞\ ➹ Performance (1)
📘\ ⚙ Maintainability (1)

Grey Divider


Remediation recommended

1. Duplicated ModifyRDSDBInstance POST logic 📘
Description
The PR adds repeated blocks that compute tenant_id and POST to
subscriptions/{tenant_id}/ModifyRDSDBInstance across multiple RDS commands, increasing maintenance
cost and risk of inconsistent updates. This should be extracted into a shared helper to satisfy the
duplication-elimination requirement.
Code

src/duplo_resource/rds.py[R184-185]

+    tenant_id = self.tenant["TenantId"]
+    self.client.post(f"subscriptions/{tenant_id}/ModifyRDSDBInstance", body)
Evidence
PR Compliance ID 4 requires extracting shared behavior into helpers instead of duplicating logic.
The modified RDS commands now repeat the same tenant_id lookup and ModifyRDSDBInstance POST
pattern in multiple places, rather than centralizing it.

src/duplo_resource/rds.py[184-185]
src/duplo_resource/rds.py[201-202]
src/duplo_resource/rds.py[244-245]
src/duplo_resource/rds.py[152-159]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`iam_auth`, `final_snapshot`, and `retention_period` duplicate the same tenant-id resolution + `ModifyRDSDBInstance` POST logic (also already present in `set_monitor_interval`). This should be centralized in a helper method (e.g., `_modify_db_instance(body)`), and each command should call the helper.

## Issue Context
Compliance requires eliminating duplicated logic to reduce drift and ensure future endpoint/parameter changes are made once.

## Fix Focus Areas
- src/duplo_resource/rds.py[141-206]
- src/duplo_resource/rds.py[233-246]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Extra tenant lookup call 🐞
Description
The updated RDS commands fetch tenant_id via self.tenant["TenantId"], which forces a tenant
discovery API call even when --tenant-id is already set. This adds an avoidable request (and
potential failure point) compared to using self.tenant_id, which can resolve directly from
self.duplo.tenantid without discovery.
Code

src/duplo_resource/rds.py[R184-185]

+    tenant_id = self.tenant["TenantId"]
+    self.client.post(f"subscriptions/{tenant_id}/ModifyRDSDBInstance", body)
Evidence
In the modified RDS commands, tenant_id is derived from self.tenant, and the injected tenant
property triggers tenant_svc.find() whenever _tenant is not cached. In contrast, the injected
tenant_id property uses self.duplo.tenantid directly when provided (no tenant lookup). The prior
implementation used update(), which calls self.endpoint(...), and tenant-scoped endpoint()
uses self.tenant_id (not self.tenant).

src/duplo_resource/rds.py[173-205]
src/duplo_resource/rds.py[233-255]
src/duplocloud/commander.py[44-66]
src/duplocloud/commander.py[94-113]
src/duplocloud/resource.py[286-312]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`iam_auth`, `final_snapshot`, and `retention_period` now compute `tenant_id` using `self.tenant["TenantId"]`. For tenant-scoped resources, accessing `self.tenant` triggers `tenant_svc.find()` when `_tenant` is unset, which adds an extra network call even when the user already provided `--tenant-id`.

## Issue Context
The tenant-scope injection already provides a `tenant_id` property that can resolve directly from `self.duplo.tenantid` without a discovery call; this is also what tenant-scoped `endpoint()` uses.

## Fix Focus Areas
- src/duplo_resource/rds.py[141-206]
- src/duplo_resource/rds.py[233-248]

## Suggested change
Replace `tenant_id = self.tenant["TenantId"]` with `tenant_id = self.tenant_id` in the modified commands (and optionally align `set_monitor_interval` similarly for consistency).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@duploctl
Copy link
Copy Markdown
Contributor

duploctl Bot commented Apr 14, 2026

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
3757 1433 38% 0% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
src/duplo_resource/rds.py 0% 🟢
TOTAL 0% 🟢

updated for commit: 062e7b5 by action🐍

@amaechiabuah amaechiabuah requested a review from kferrone April 17, 2026 11:09
Comment thread src/duplo_resource/rds.py Outdated
@amaechiabuah amaechiabuah requested a review from kferrone April 20, 2026 18:02
Copy link
Copy Markdown
Collaborator

@kferrone kferrone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@kferrone kferrone merged commit afcc646 into main Apr 24, 2026
7 checks passed
@kferrone kferrone deleted the fix/rds-modify-endpoint branch April 24, 2026 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants