Skip to content

chore: rename ExpandRegions - #120

Merged
InftyAI-Agent merged 2 commits into
InftyAI:mainfrom
kerthcet:cleanup/renaming-expandregions
Sep 24, 2026
Merged

InftyAI-Agent merged 2 commits into
InftyAI:mainfrom
kerthcet:cleanup/renaming-expandregions

Conversation

@kerthcet

@kerthcet kerthcet commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

What this PR does / why we need it

Which issue(s) this PR fixes

Fixes #

Special notes for your reviewer

Does this PR introduce a user-facing change?


Summary by CodeRabbit

  • Bug Fixes
    • AWS region sweeps now account for each pool’s declared regions, including pools that leave regions unspecified. This helps ensure the sweep covers the regions available to placement and falls back to provisioned regions if region lookup fails.
  • Documentation
    • Updated provider and architecture documentation to consistently describe region resolution.

Signed-off-by: kerthcet <kerthcet@gmail.com>
Copilot AI lite review requested due to automatic review settings September 24, 2026 12:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@InftyAI-Agent InftyAI-Agent added needs-triage Indicates an issue or PR lacks a label and requires one. needs-priority Indicates a PR lacks a label and requires one. do-not-merge/needs-kind Indicates a PR lacks a label and requires one. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 24, 2026
@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

Next included review available in 14 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 2fe617ed-3f2f-4488-98f5-f838a2e2ae32

📥 Commits

Reviewing files that changed from the base of the PR and between 036dc41 and 1570574.

📒 Files selected for processing (1)
  • pkg/provider/aws/aws.go

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 5238cbe9-0741-4da8-b463-e8ff261b8386

📥 Commits

Reviewing files that changed from the base of the PR and between e68b713 and 036dc41.

📒 Files selected for processing (17)
  • api/v1alpha1/nodepool_types.go
  • cmd/main.go
  • config/crd/bases/nebula.inftyai.com_nodepools.yaml
  • docs/add-a-provider.md
  • docs/architecture.md
  • internal/controller/nodeclaim_controller_test.go
  • internal/controller/pod_placement_controller_test.go
  • internal/controller/pod_placement_helpers.go
  • pkg/provider/aws/aws.go
  • pkg/provider/aws/aws_test.go
  • pkg/provider/catalog/base.go
  • pkg/provider/fake/fake.go
  • pkg/provider/fake/fake_test.go
  • pkg/provider/modal/modal.go
  • pkg/provider/modal/modal_test.go
  • pkg/provider/provider.go
  • pkg/vnode/handler_test.go
💤 Files with no reviewable changes (1)
  • internal/controller/pod_placement_controller_test.go

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The provider region-resolution method is renamed to ResolveRegions. The AWS region source now passes per-NodePool region declarations to the adapter, which resolves each declaration during the sweep.

Changes

Provider region resolution

Layer / File(s) Summary
Rename the provider API and update its references
pkg/provider/provider.go, pkg/provider/aws/aws.go, pkg/provider/fake/fake.go, pkg/provider/modal/modal.go, pkg/provider/catalog/base.go, pkg/provider/*/*_test.go, pkg/vnode/handler_test.go, internal/controller/nodeclaim_controller_test.go, api/v1alpha1/nodepool_types.go, config/crd/bases/nebula.inftyai.com_nodepools.yaml, docs/add-a-provider.md, docs/architecture.md
The provider interface method and its implementations are renamed from ExpandRegions to ResolveRegions. Associated tests, comments, and documentation are updated to use the new name.
Resolve per-pool declarations in the AWS sweep
pkg/provider/aws/aws.go, pkg/provider/aws/aws_test.go
RegionSource now returns [][]string containing per-NodePool declarations. The AWS sweep expands each declaration, including nil declarations, and tests verify sweep coverage against each pool’s resolved regions.
Pass declarations from pools and update placement
cmd/main.go, internal/controller/pod_placement_helpers.go, internal/controller/pod_placement_controller_test.go
awsRegionSource returns unexpanded declarations for AWS pools. Placement calls ResolveRegions. The test comparing placement expansion with AWS sweep expansion is removed.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant awsRegionSource
  participant AWSProvider
  participant expandDeclared
  AWSProvider->>awsRegionSource: Read per-pool declarations
  awsRegionSource-->>AWSProvider: Return [][]string
  AWSProvider->>expandDeclared: Resolve each declaration
  expandDeclared-->>AWSProvider: Return concrete regions
Loading

Merge Risk: ⚪ Minimal · up to 036dc

The region-resolution rename and AWS sweep change appear mergeable after normal checks; no actionable merge-blocking risk is identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 70.83% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 13 files. (3 skipped:… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary change: renaming the ExpandRegions API to ResolveRegions.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 70.83% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 13 files. (3 skipped: 3 unsupported.)

✨ Finishing Touches 💡 1
🧪 Generate unit tests (beta)
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Signed-off-by: kerthcet <kerthcet@gmail.com>
Copilot AI review requested due to automatic review settings September 24, 2026 13:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@kerthcet

Copy link
Copy Markdown
Member Author

/lgtm
/kind cleanup

@InftyAI-Agent InftyAI-Agent added lgtm Looks good to me, indicates that a PR is ready to be merged. cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed do-not-merge/needs-kind Indicates a PR lacks a label and requires one. labels Sep 24, 2026

@InftyAI-Agent InftyAI-Agent left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approved: PR has both lgtm and approved labels

@InftyAI-Agent
InftyAI-Agent merged commit 7bf4809 into InftyAI:main Sep 24, 2026
25 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Looks good to me, indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a label and requires one. needs-triage Indicates an issue or PR lacks a label and requires one.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants