feat(network): add Europe region mappings for all cloud providers #149
Conversation
- Add GCP Europe regions (europe-west1-6, europe-north1) - Add AWS Europe regions (eu-west-1/2/3, eu-central-1/2, eu-north-1) - Add Azure Europe regions (westeurope, northeurope, uksouth, ukwest, etc.) - Update RegionToSuperRegion to map EU regions to SuperRegionEUWest/EUEast - Add tests for IsSuperRegion and IsSuperRegionLocal functions - Update existing tests to reflect new Europe region support
📝 WalkthroughWalkthroughAdded six EU region constants and three super-region constants, expanded region-to-super-region and provider region mappings, introduced IsSuperRegion and IsSuperRegionLocal helpers, and extended test coverage for new regions and helpers across the network package. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (2)
network/super_region_test.go (1)
8-41: Consider extendingTestGetSuperRegionto cover EU regions.The existing test only covers US regions. Since you've added EU region-to-super-region mappings (
RegionEUWest1/2/3→SuperRegionEUWest,RegionEUEast1/2/3→SuperRegionEUEast), consider adding test cases for EU regions to ensure complete coverage.Suggested additional test cases
tests := []struct { name string region Region expectedSuper SuperRegion shouldError bool }{ {"USCentral1", RegionUSCentral1, "usc", false}, {"USCentral2", RegionUSCentral2, "usc", false}, {"USWest1", RegionUSWest1, "usw", false}, {"USWest2", RegionUSWest2, "usw", false}, {"USEast1", RegionUSEast1, "use", false}, {"USEast2", RegionUSEast2, "use", false}, + {"EUWest1", RegionEUWest1, "euw", false}, + {"EUWest2", RegionEUWest2, "euw", false}, + {"EUWest3", RegionEUWest3, "euw", false}, + {"EUEast1", RegionEUEast1, "eue", false}, + {"EUEast2", RegionEUEast2, "eue", false}, + {"EUEast3", RegionEUEast3, "eue", false}, {"GlobalShouldError", RegionGlobal, "", true}, }network/ipv6_test.go (1)
602-628: LGTM with a minor nit: Update the assertion message.The test cases correctly verify the new EU region mappings. However, the assertion message on line 625 says "should default to global" which is now misleading for EU region test cases that explicitly expect non-global regions.
Optional: Update the assertion message
- assert.Equal(t, tc.expected, result, "Non-US region %s should default to global", tc.region, tc.expected) + assert.Equal(t, tc.expected, result, "Region %s should map to %v", tc.region, tc.expected)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
network/ipv6.gonetwork/ipv6_test.gonetwork/super_region_test.go
🧰 Additional context used
🧬 Code graph analysis (3)
network/super_region_test.go (1)
network/ipv6.go (2)
IsSuperRegion(82-90)IsSuperRegionLocal(93-95)
network/ipv6_test.go (1)
network/ipv6.go (4)
RegionEUWest1(63-63)RegionEUWest2(64-64)RegionEUWest3(65-65)RegionEUEast1(59-59)
network/ipv6.go (1)
network/dns.go (3)
CloudProvider(17-17)CloudProviderAWS(20-20)CloudProviderGCP(21-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
network/super_region_test.go (3)
127-139: LGTM!The expected count and hostname verification correctly reflect the addition of EU super-regions (
euwandeue).
148-173: LGTM!Good test coverage for
IsSuperRegion, including edge cases for case sensitivity and the distinction between regular super-regions and the local super-region.
175-196: LGTM!Comprehensive test coverage for
IsSuperRegionLocal, including proper verification of case sensitivity.network/ipv6.go (6)
43-66: LGTM!The new EU region constants are well-organized with clear sequential hex values and logical grouping.
68-79: LGTM!The new super-region constants follow a consistent naming pattern.
81-95: LGTM!Clean implementation with clear separation between production super-regions and the local development super-region.
97-109: Verify if EU regions should be added toProductionRegions.The PR adds comprehensive EU region mappings, but
ProductionRegionscurrently only includes US regions. If EU regions are intended to be production-ready, consider updating this map to include them. If this is intentional (EU regions not yet deployed), no change is needed.
143-168: Verify cross-provider region mapping consistency for Frankfurt and Zurich.There are potential inconsistencies in how the same geographic locations map to internal regions across providers:
Location GCP Internal Region AWS Internal Region Frankfurt europe-west3 RegionEUWest3eu-central-1 RegionEUEast1Zurich europe-west6 RegionEUWest1eu-central-2 RegionEUEast2If the intent is to have consistent internal region assignment for the same physical location across providers (for cross-cloud routing or failover), these mappings may need adjustment. If the mappings are intentional based on each provider's internal region groupings, this is fine as-is.
184-189: LGTM!The EU region-to-super-region mappings are complete and consistent.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
network/ipv6_test.go (1)
602-627: LGTM! Test coverage for EU region mappings looks good.The test additions appropriately cover representative EU regions from all three cloud providers (GCP, AWS, Azure) and verify they map to the correct
RegionEU*constants. The updated assertion message accurately reflects that the test now validates specific region mappings rather than just global fallback behavior.One optional consideration: the function name
TestGetRegionFallbackis now slightly misleading since it tests both EU region mappings (which aren't fallbacks) and actual global fallback cases. If you'd like, you could rename it to something likeTestGetRegionNonUSor split into separate test functions.network/super_region_test.go (1)
154-179: Good test coverage forIsSuperRegion.The test cases comprehensively cover:
- All valid super regions (usc, usw, use, euw, eue)
- Case sensitivity ("USC" correctly returns false)
- Edge cases (empty string, invalid values)
- Distinction from local super region ("l" returns false)
One minor note: using
tt.valas the test name means the empty string case (line 165) will have an empty subtest name, which can make test output harder to read. Consider using a descriptive name.♻️ Optional: Use descriptive test names
func TestIsSuperRegion(t *testing.T) { tests := []struct { + name string val string expected bool }{ - {"usc", true}, - {"usw", true}, - {"use", true}, - {"euw", true}, - {"eue", true}, - {"l", false}, - {"", false}, - {"invalid", false}, - {"USC", false}, - {"us-central", false}, + {"valid_usc", "usc", true}, + {"valid_usw", "usw", true}, + {"valid_use", "use", true}, + {"valid_euw", "euw", true}, + {"valid_eue", "eue", true}, + {"local_super_region", "l", false}, + {"empty_string", "", false}, + {"invalid_string", "invalid", false}, + {"uppercase_USC", "USC", false}, + {"wrong_format", "us-central", false}, } for _, tt := range tests { - t.Run(tt.val, func(t *testing.T) { + t.Run(tt.name, func(t *testing.T) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
network/ipv6_test.gonetwork/super_region_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
network/super_region_test.go (1)
network/ipv6.go (8)
RegionEUWest1(63-63)RegionEUWest2(64-64)RegionEUWest3(65-65)RegionEUEast1(59-59)RegionEUEast2(60-60)RegionEUEast3(61-61)IsSuperRegion(82-90)IsSuperRegionLocal(93-95)
network/ipv6_test.go (1)
network/ipv6.go (4)
RegionEUWest1(63-63)RegionEUWest2(64-64)RegionEUWest3(65-65)RegionEUEast1(59-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
network/super_region_test.go (3)
21-26: LGTM!Good addition of test cases for all six EU regions, correctly mapping to the "euw" and "eue" super regions. The test structure maintains consistency with the existing US region cases.
133-144: LGTM!The expected count and hostname set are correctly updated to include the two new EU super regions ("euw" and "eue").
181-202: Good test coverage forIsSuperRegionLocal.The test cases effectively verify that only the exact "l" value is recognized as local, with proper coverage for case sensitivity and distinction from other super regions.
Same minor observation as above regarding test naming for the empty string case.
Summary
Adds provider-specific Europe region mappings to the
Regionsmap, enabling proper region resolution for GCP, AWS, and Azure Europe regions.Changes
Region Mappings Added
GCP Europe regions:
europe-west1→ RegionEUWest1 (Belgium)europe-west2→ RegionEUWest2 (London)europe-west3→ RegionEUWest3 (Frankfurt)europe-west4→ RegionEUWest1 (Netherlands)europe-west6→ RegionEUWest1 (Zurich)europe-north1→ RegionEUEast1 (Finland)AWS Europe regions:
eu-west-1→ RegionEUWest1 (Ireland)eu-west-2→ RegionEUWest2 (London)eu-west-3→ RegionEUWest3 (Paris)eu-central-1→ RegionEUEast1 (Frankfurt)eu-central-2→ RegionEUEast2 (Zurich)eu-north-1→ RegionEUEast3 (Stockholm)Azure Europe regions:
westeurope→ RegionEUWest1 (Netherlands)northeurope→ RegionEUWest2 (Ireland)uksouth→ RegionEUWest2 (UK South)ukwest→ RegionEUWest3 (UK West)germanywestcentral→ RegionEUEast1 (Frankfurt)swedencentral→ RegionEUEast2 (Sweden)norwayeast→ RegionEUEast3 (Norway)francecentral→ RegionEUWest3 (France)Other Changes
RegionToSuperRegionto include EU region → super region mappingsTestIsSuperRegionandTestIsSuperRegionLocaltest functionsTesting
go test ./network/...All tests pass.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.