Support regions/vpc-availability endpoints#880
Support regions/vpc-availability endpoints#880dawiddzhafarov wants to merge 6 commits intolinode:mainfrom
Conversation
| }, | ||
| } | ||
|
|
||
| assert.Equal(t, expected, availability, "Expected region vpc availability list to match expected data") |
There was a problem hiding this comment.
Let me know if you prefer one-by-one assertions on each of the returned fields instead of a single object comparison.
| Available bool `json:"available"` | ||
| } | ||
|
|
||
| // RegionVPCAvailability represents a linode region vpc availability object |
There was a problem hiding this comment.
I saw that usually the methods end in dots, but the comments for structs or vars don't. Is it intentional?
regions_availability.go
Outdated
| func (c *Client) ListRegionsVPCAvailability(ctx context.Context, opts *ListOptions) ([]RegionVPCAvailability, error) { | ||
| e := "regions/vpc-availability" | ||
|
|
||
| endpoint, err := generateListCacheURL(e, opts) |
There was a problem hiding this comment.
Still don't know if those endpoints have caching enabled and not sure how do I check it?
There was a problem hiding this comment.
The cache is local and built in the SDK, not involving API. We enabled the cache for some mostly static API responses, such as list Linode instance types, in the past.
However, since the cache is an implicit behavior and might confuse users, plus the VPC is a relatively new features being gradually added to different data centers, which can cause changes in the API response, I think we might want to keep as a simple non-cached endpoint.
There was a problem hiding this comment.
Thanks, that makes sense.
I have removed caching from those endpoints.
dawiddzhafarov
requested review from
Copilot,
psnoch-akamai and
vshanthe
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
This PR adds support for VPC availability endpoints in regions, enabling clients to query which regions support VPCs and their available IPv6 prefix lengths.
Changes:
- Added
RegionVPCAvailabilitystruct and two new API methods:ListRegionsVPCAvailabilityandGetRegionVPCAvailability - Implemented comprehensive unit tests with fixture data for both list and get operations
- Added integration tests with recorded API responses
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| regions_availability.go | Implements new RegionVPCAvailability struct and API methods with caching support |
| test/unit/region_test.go | Adds unit tests for listing and retrieving VPC availability data |
| test/unit/fixtures/regions_vpc_availability_list.json | Fixture data for list VPC availability endpoint |
| test/unit/fixtures/region_vpc_availability_get.json | Fixture data for get VPC availability endpoint |
| test/integration/regionsavailability_test.go | Integration tests for VPC availability endpoints |
| test/integration/fixtures/TestRegionsVPCAvailability_List.yaml | Recorded API response for list operation |
| test/integration/fixtures/TestRegionVPCAvailability_Get.yaml | Recorded API response for get operation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "available": true, | ||
| "available_ipv6_prefix_lengths": [ | ||
| 52, | ||
| 60 |
There was a problem hiding this comment.
The IPv6 prefix length value 60 is inconsistent with the list fixture and integration test fixture, which both use 52. For test consistency, this should be 52 unless testing different scenarios is intentional.
| 60 | |
| 52 |
| expected := &linodego.RegionVPCAvailability{ | ||
| Region: "us-east", | ||
| Available: true, | ||
| AvailableIPV6PrefixLengths: []int{52, 60}, |
There was a problem hiding this comment.
The expected IPv6 prefix length [52, 60] doesn't match the fixture data being tested against. The fixture 'region_vpc_availability_get.json' contains [52, 60], but this differs from the integration test fixture which uses [52]. This inconsistency may indicate incorrect test expectations or fixture data.
| AvailableIPV6PrefixLengths: []int{52, 60}, | |
| AvailableIPV6PrefixLengths: []int{52}, |
dawiddzhafarov
force-pushed
the
feature/TPT-4108-linodego-support-regions-vpc-availability-endpoints
branch
3 times, most recently
from
b2109d6 to
80e4f55
Compare
There was a problem hiding this comment.
The output of those fixtures were recorded in the devcloud env rather than prod. This is because I don't have the token yet. But if I understand correctly, if someone run make fixtures for those tests, the output would differ greatly. That's why @mgwoj generated those output fixtures for production (as he has access) so I could use them here already, and future make fixtures would not generate such big diffs.
Should I use the fixtures obtained from prod env by Michał already?
There was a problem hiding this comment.
I have gained the access to the prod env and regenerated test data based on that env.
dawiddzhafarov
force-pushed
the
feature/TPT-4108-linodego-support-regions-vpc-availability-endpoints
branch
from
80e4f55 to
2aa4f20
Compare
2 participants
📝 Description
Add support for region vpc availability endpoints
✔️ How to Test
To run all unit and integration tests:
make test-unitmake test-intTo run only newly added unit and integration tests:
make TEST_ARGS="-run TestListRegionsVPCAvailability" test-unitmake TEST_ARGS="-run TestGetRegionVPCAvailability" test-unitmake TEST_ARGS="-run TestRegionsVPCAvailability_List" test-intmake TEST_ARGS="-run TestRegionVPCAvailability_Get" test-int