Skip to content

fix: cloudfront apply crashes with KeyError on metadata#249

Merged
kferrone merged 10 commits intomainfrom
fix/cloudfront-apply
Apr 24, 2026
Merged

fix: cloudfront apply crashes with KeyError on metadata#249
kferrone merged 10 commits intomainfrom
fix/cloudfront-apply

Conversation

@amaechiabuah
Copy link
Copy Markdown
Collaborator

Describe Changes

  • Overrode apply() on DuploCloudFront since CloudFront uses server-assigned distribution IDs, not names — the inherited V3 apply() calls name_from_body() which expects body["metadata"]["name"], causing a KeyError
  • New logic: if body contains Id → update, otherwise → create

Link to Issues

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

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

Task linked: DUPLO-41929 CloudFront: All commands failed

@duploctl
Copy link
Copy Markdown
Contributor

duploctl Bot commented Apr 1, 2026

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
3722 1433 39% 0% 🟢

New Files

No new covered files...

Modified Files

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

updated for commit: 51cded0 by action🐍

@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Fix CloudFront apply crash with ID-based create-or-update logic

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Override apply() method in DuploCloudFront to handle server-assigned distribution IDs
• Implement ID-based create-or-update logic instead of name-based lookup
• Fix KeyError: 'metadata' crash when applying CloudFront distributions
Diagram
flowchart LR
  A["CloudFront apply request"] --> B{"Body contains Id?"}
  B -->|Yes| C["Call update method"]
  B -->|No| D["Call create method"]
  C --> E["Updated distribution"]
  D --> F["Created distribution"]
Loading

Grey Divider

File Changes

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

Document CloudFront apply KeyError fix

• Added entry documenting the fix for cloudfront apply KeyError crash
• Noted the override of apply with ID-based create-or-update logic

CHANGELOG.md


2. src/duplo_resource/cloudfront.py 🐞 Bug fix +23/-0

Override apply method with ID-based logic

• Added new apply() method override to DuploCloudFront class
• Implements ID-based logic: if body contains Id field, call update(), otherwise call create()
• Includes comprehensive docstring with usage examples and parameter documentation
• Resolves KeyError crash from inherited V3 apply() that expected body["metadata"]["name"]

src/duplo_resource/cloudfront.py


Grey Divider

Qodo Logo

@amaechiabuah amaechiabuah requested a review from kferrone April 6, 2026 12:50
Comment thread src/duplo_resource/cloudfront.py Outdated
Returns:
resource: The created or updated distribution.
"""
if body.get("Id"):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This doesn't look right. If I made a yaml file in a repo and then made an action that will "apply" that yaml, then the ID would never be in that file unless you were to update the file after creating it. But then you would not be able to have multiple instances of the same yaml because of that id.

So instead, we need a reliable name_from_body method that uses the comment from the cloudfront body. The comment is the name for a cloudfront, this is actually the case in AWS itself too.

Then the find MUST use the comment or ID to find something. You can review how the tenant.find() method works to understand the flow for a name or id style lookup.

When the find is able to use either a name or id then something using the find can use either the id if given or the name second.

@amaechiabuah amaechiabuah requested a review from kferrone April 17, 2026 12:58
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 4405928 into main Apr 24, 2026
7 checks passed
@kferrone kferrone deleted the fix/cloudfront-apply branch April 24, 2026 15:49
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.

5 participants