fix(restore-with-files): Add sanitized site config to agent payload#6441
fix(restore-with-files): Add sanitized site config to agent payload#6441regdocs wants to merge 2 commits into
Conversation
|
| Filename | Overview |
|---|---|
| press/agent.py | Adds sanitized site config to the restore-site agent payload; contains two runtime-breaking bugs: wrong import path (press.press.utils doesn't exist) and missing initialisation of sanitized_config_content before the conditional block causes a NameError on every restore without a config file. |
Sequence Diagram
sequenceDiagram
participant Press as Press (agent.py)
participant RF as Remote File (DocType)
participant Utils as press.utils.sanitize_config
participant Agent as Agent Job (Restore Site)
Press->>Press: restore_site(site)
Press->>Press: check_space_on_server_for_restore()
Press->>RF: get_doc("Remote File", remote_database_file)
RF-->>Press: database_link
Press->>RF: get_doc("Remote File", remote_public_file)
RF-->>Press: public_link
Press->>RF: get_doc("Remote File", remote_private_file)
RF-->>Press: private_link
alt site.remote_config_file exists
Press->>RF: get("Remote File", remote_config_file).get_content()
RF-->>Press: config dict (parsed JSON)
Press->>Utils: sanitize_config(config_content)
Utils-->>Press: sanitized_config (blacklisted keys removed)
Press->>Press: "sanitized_config.update({maintenance_mode: 0})"
end
Press->>Agent: create_agent_job("Restore Site", data incl. sanitized_config_content)
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
press/agent.py:176
The import path `press.press.utils` does not exist — there is no `press/press/utils.py` file in the repository. `sanitize_config` lives in `press/utils/__init__.py`, so the correct import is `press.utils`. This will raise an `ImportError` at the start of every `restore_site` call, completely breaking site restoration.
```suggestion
from press.utils import sanitize_config
```
### Issue 2 of 3
press/agent.py:180
`sanitized_config_content` is only assigned inside the `if site.remote_config_file:` block, but it is referenced unconditionally in the `data` dict below. When `site.remote_config_file` is absent (the common case for restores that don't include a config file), Python will raise `NameError: name 'sanitized_config_content' is not defined`, breaking every such restore. The pattern used for `database_link`, `public_link`, and `private_link` — initialise to `None` before the conditional — should be applied here too.
```suggestion
public_link, private_link, database_link, sanitized_config_content = None, None, None, None
```
### Issue 3 of 3
press/agent.py:188
The other three remote file lookups all use `frappe.get_doc(...)`, but this one uses `frappe.get(...)`. While both may resolve identically in some Frappe versions, using the inconsistent form can introduce subtle runtime differences and makes the code harder to follow. Aligning with the established pattern in the same function reduces that risk.
```suggestion
config_content = frappe.get_doc("Remote File", site.remote_config_file).get_content()
```
Reviews (1): Last reviewed commit: "fix(restore-with-files): Sanitize site c..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (12.50%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #6441 +/- ##
===========================================
- Coverage 49.79% 49.72% -0.07%
===========================================
Files 945 945
Lines 78390 78397 +7
Branches 355 355
===========================================
- Hits 39031 38980 -51
- Misses 39336 39394 +58
Partials 23 23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5303609 to
5ec67e8
Compare
5ec67e8 to
f8f30d1
Compare
|
related? #2213 |
|
@ph0ton yeah, thanks for the pointer |
Resolves #2032
Needs to be paired with frappe/agent#509.