feat(aws): Support instance creation from AMI id - #871
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAWS Fedora, RHEL, RHEL AI, and Windows host creation commands now accept optional AMI IDs. Providers resolve explicit IDs through EC2 lookup and retain existing name-based resolution when no ID is supplied. ChangesCustom AMI deployment
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CLI
participant HostCreate
participant AMIService
participant AWS_EC2
CLI->>HostCreate: provide optional AMI ID
HostCreate->>AMIService: resolve AMI
AMIService->>AWS_EC2: lookup by image ID or name
AWS_EC2-->>AMIService: AMI result
AMIService-->>HostCreate: selected AMI
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/provider/aws/action/snc/snc.go`:
- Around line 183-194: Declare an error variable in deploy before the r.ami
conditional so it is in scope for the AMI fallback. Keep the existing ami, err =
amiSVC.GetAMIByName assignment in the else branch, avoiding := so the outer ami
and err variables are reused rather than shadowed.
In `@pkg/provider/aws/data/ami.go`:
- Around line 168-170: Update IsAMIIDValid’s DescribeImages error handling to
return false, nil only for an error that confirms the AMI is unavailable;
propagate all other AWS lookup errors to the caller. Preserve the existing
valid-image and unavailable-AMI behavior while retaining the original error for
access, throttling, network, and configuration failures.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c20beb59-248f-421f-b4b2-5ea693dbbe4d
📒 Files selected for processing (4)
cmd/mapt/cmd/aws/services/snc.gopkg/provider/aws/action/snc/snc.gopkg/provider/aws/data/ami.gopkg/target/service/snc/api.go
| if err != nil { | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
rg -n -C4 'DescribeImages|return false, nil|IsAMIIDValid' pkg/provider/aws/data/ami.go pkg/provider/aws/action/snc/snc.goRepository: redhat-developer/mapt
Length of output: 4688
🏁 Script executed:
#!/usr/bin/env bash
python3 - <<'PY'
from pathlib import Path
import re
ami = Path("pkg/provider/aws/data/ami.go").read_text()
m = re.search(r'func IsAMIIDValid\(.*?\n(?:^(?:\s*.+\n)*)', ami, re.S | re.M)
if not m:
print("IsAMIIDValid body not found")
raise SystemExit(0)
body = m.group(0)
print("IsAMIIDValid body:")
print(body)
print("contains DescribeImages call:", "DescribeImages(ctx, &ec2.DescribeImagesInput{" in body)
print("error block:", bool(re.search(r'if err != nil \{\s*return false, nil\s*\}', body)))
print("returns valid check:", bool(re.search(r'return result != nil && len\(result\.Images\) > 0, nil', body)))
PYRepository: redhat-developer/mapt
Length of output: 2264
Propagate unexpected DescribeImages errors.
IsAMIIDValid converts all AWS image lookup failures to false, nil, so the caller reports access, throttling, network, and config issues as “AMI ... could not be found in region.” Only return false, nil when the error indicates the AMI is genuinely unavailable; otherwise return the error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/provider/aws/data/ami.go` around lines 168 - 170, Update IsAMIIDValid’s
DescribeImages error handling to return false, nil only for an error that
confirms the AMI is unavailable; propagate all other AWS lookup errors to the
caller. Preserve the existing valid-image and unavailable-AMI behavior while
retaining the original error for access, throttling, network, and configuration
failures.
adrianriobo
left a comment
adrianriobo
left a comment
There was a problem hiding this comment.
Can we avoid this in service targets (i.e. kind and openshift-snc)?
|
Also did you try it? using the ghcr image from this PR? |
|
Will remove for kind and openshift-snc and test with some existing AMI (which even request some decription key) |
|
let me know once you do it |
Signed-off-by: Alberto Fanjul <afanjula@redhat.com>
2 participants
fixes #626