Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: vlans is now required in the Metal API for shared connections #605

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

ctreatma
Copy link
Contributor

@ctreatma ctreatma commented Mar 1, 2024

The Metal API was recently updated to change the behavior of shared interconnections as follows:

  • VLANs must be specified at creation for shared connections
  • VLANs cannot be updated for shared connections

This updates the connection resource to align with the new API behavior so that users can catch configuration errors earlier.

Note this rolls back #273, which was written to address #222.

This also includes a fix for #595

@ctreatma ctreatma force-pushed the shared-connection-vlans branch from af55dba to db16b50 Compare March 1, 2024 20:27
@ctreatma ctreatma marked this pull request as ready for review March 1, 2024 20:29
@ctreatma ctreatma requested review from t0mk and a team as code owners March 1, 2024 20:29
@ctreatma ctreatma changed the title fix: vlans is now required in the Metal APi for shared connections fix: vlans is now required in the Metal API for shared connections Mar 1, 2024
@ctreatma ctreatma force-pushed the shared-connection-vlans branch from db16b50 to e82ae66 Compare March 1, 2024 20:38
@displague
Copy link
Member

displague commented Mar 1, 2024

@ctreatma can we pick this up in the PR: (or another)

@displague
Copy link
Member

displague commented Mar 1, 2024

Putting blinders on to the following which are related but would are a more involved than this bug-fix PR should delve into

@ctreatma ctreatma force-pushed the shared-connection-vlans branch from e82ae66 to 287883a Compare March 1, 2024 21:10
@ctreatma ctreatma force-pushed the shared-connection-vlans branch from 287883a to 5d89a42 Compare March 1, 2024 21:11
@@ -19,6 +19,7 @@ var (
{2 * giga, "2Gbps"},
{5 * giga, "5Gbps"},
{10 * giga, "10Gbps"},
{100 * giga, "100Gbps"},
Copy link
Contributor Author

@ctreatma ctreatma Mar 1, 2024

Choose a reason for hiding this comment

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

I don't love hiding this fix in an unrelated PR but it's so tiny.

(Side note, though, I have a bigger change locally to remove this list from the provider and just convert arbitrary speed values from int <=> string and let the API decide what's valid or not.)

Copy link
Member

@displague displague Mar 4, 2024

Choose a reason for hiding this comment

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

No good deed goes unpunished..

=== RUN   TestSpeedConversion
    speed_test.go:26: Expected error converting invalid speed string to uint, got: 100000000000
    speed_test.go:31: Expected error converting invalid speed uint to string, got: 100Gbps
--- FAIL: TestSpeedConversion (0.00s)

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to merge this since we have a few dependent PRs now in the works (a fix for this test with your bigger local changes and #607)

Copy link
Member

Choose a reason for hiding this comment

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

I've opened an issue to follow-up on this: #609

Copy link
Member

@0xch4z 0xch4z left a comment

Choose a reason for hiding this comment

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

LGTM

0xch4z added a commit to 0xch4z/terraform-provider-equinix that referenced this pull request Mar 3, 2024
Depends on equinix#605 and some API spec updates

Changes:
- equinix_metal_connection now uses equinix-sdk-go instead of packngo
- "vrfs" field added to equinix_metal_connection resource + datasource
- "contact_email" can be updated without recreation
- "redundancy" updates cause recreation

Breaking changes:
- Updating "redundancy" forces recreate (this goes along with the
  previous change to force new on "vlans" changes, if a new vlan/vrf
  cannot be added then switching between primary and redundant should
  not be allowed)
@displague displague merged commit a09e337 into main Mar 4, 2024
4 of 6 checks passed
@displague displague deleted the shared-connection-vlans branch March 4, 2024 14:42
ctreatma added a commit that referenced this pull request Mar 4, 2024
Previously, we maintained a list of valid connection speed values in the
terraform provider in order to steer users towards selecting a valid
connection speed. As shown by #595, validating values in the provider
has the potential to break when the list of connection speed values
supported by the API changes.

This updates the code that converts speed values between integer and
string representations (i.e., the code that knows that `"50Mbps"` is the
same as `500000000`) to convert (roughly) arbitrary values. This means
that new values are immediately "supported" by the provider, but it also
means that invalid values will not be flagged until apply.

The test change in this PR is sufficient to resolve the issue introduced
by merging #605 without any other change to provider behavior.

Closes #609
0xch4z added a commit to 0xch4z/terraform-provider-equinix that referenced this pull request Mar 5, 2024
Depends on equinix#605 and some API spec updates

Changes:
- equinix_metal_connection now uses equinix-sdk-go instead of packngo
- "vrfs" field added to equinix_metal_connection resource + datasource
- "contact_email" can be updated without recreation
- "redundancy" updates cause recreation

Breaking changes:
- Updating "redundancy" forces recreate (this goes along with the
  previous change to force new on "vlans" changes, if a new vlan/vrf
  cannot be added then switching between primary and redundant should
  not be allowed)
0xch4z added a commit to 0xch4z/terraform-provider-equinix that referenced this pull request Mar 5, 2024
Depends on equinix#605 and some API spec updates

Changes:
- equinix_metal_connection now uses equinix-sdk-go instead of packngo
- "vrfs" field added to equinix_metal_connection resource + datasource
- "contact_email" can be updated without recreation
- "redundancy" updates cause recreation

Breaking changes:
- Updating "redundancy" forces recreate (this goes along with the
  previous change to force new on "vlans" changes, if a new vlan/vrf
  cannot be added then switching between primary and redundant should
  not be allowed)
0xch4z added a commit to 0xch4z/terraform-provider-equinix that referenced this pull request Mar 5, 2024
Depends on equinix#605 and some API spec updates

Changes:
- equinix_metal_connection now uses equinix-sdk-go instead of packngo
- "vrfs" field added to equinix_metal_connection resource + datasource
- "contact_email" can be updated without recreation
- "redundancy" updates cause recreation

Breaking changes:
- Updating "redundancy" forces recreate (this goes along with the
  previous change to force new on "vlans" changes, if a new vlan/vrf
  cannot be added then switching between primary and redundant should
  not be allowed)
0xch4z added a commit to 0xch4z/terraform-provider-equinix that referenced this pull request Mar 8, 2024
Depends on equinix#605 and some API spec updates

Changes:
- equinix_metal_connection now uses equinix-sdk-go instead of packngo
- "vrfs" field added to equinix_metal_connection resource + datasource
- "contact_email" can be updated without recreation
- "redundancy" updates cause recreation

Breaking changes:
- Updating "redundancy" forces recreate (this goes along with the
  previous change to force new on "vlans" changes, if a new vlan/vrf
  cannot be added then switching between primary and redundant should
  not be allowed)
0xch4z added a commit to 0xch4z/terraform-provider-equinix that referenced this pull request Mar 8, 2024
Depends on equinix#605 and some API spec updates

Changes:
- equinix_metal_connection now uses equinix-sdk-go instead of packngo
- "vrfs" field added to equinix_metal_connection resource + datasource
- "contact_email" can be updated without recreation
- "redundancy" updates cause recreation

Breaking changes:
- Updating "redundancy" forces recreate (this goes along with the
  previous change to force new on "vlans" changes, if a new vlan/vrf
  cannot be added then switching between primary and redundant should
  not be allowed)
0xch4z added a commit to 0xch4z/terraform-provider-equinix that referenced this pull request Mar 8, 2024
Depends on equinix#605 and some API spec updates

Changes:
- equinix_metal_connection now uses equinix-sdk-go instead of packngo
- "vrfs" field added to equinix_metal_connection resource + datasource
- "contact_email" can be updated without recreation
- "redundancy" updates cause recreation

Breaking changes:
- Updating "redundancy" forces recreate (this goes along with the
  previous change to force new on "vlans" changes, if a new vlan/vrf
  cannot be added then switching between primary and redundant should
  not be allowed)
0xch4z added a commit to 0xch4z/terraform-provider-equinix that referenced this pull request Mar 8, 2024
Depends on equinix#605 and some API spec updates

Changes:
- equinix_metal_connection now uses equinix-sdk-go instead of packngo
- "vrfs" field added to equinix_metal_connection resource + datasource
- "contact_email" can be updated without recreation
- "redundancy" updates cause recreation

Breaking changes:
- Updating "redundancy" forces recreate (this goes along with the
  previous change to force new on "vlans" changes, if a new vlan/vrf
  cannot be added then switching between primary and redundant should
  not be allowed)
0xch4z added a commit to 0xch4z/terraform-provider-equinix that referenced this pull request Mar 8, 2024
Depends on equinix#605 and some API spec updates

Changes:
- equinix_metal_connection now uses equinix-sdk-go instead of packngo
- "vrfs" field added to equinix_metal_connection resource + datasource
- "contact_email" can be updated without recreation
- "redundancy" updates cause recreation

Breaking changes:
- Updating "redundancy" forces recreate (this goes along with the
  previous change to force new on "vlans" changes, if a new vlan/vrf
  cannot be added then switching between primary and redundant should
  not be allowed)
ctreatma pushed a commit that referenced this pull request Mar 11, 2024
Depends on
#605 and
equinix/equinix-sdk-go#40

Changes:
- equinix_metal_connection now uses equinix-sdk-go instead of packngo
- "vrfs" field added to equinix_metal_connection resource + datasource
- "contact_email" can be updated without recreation
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.

3 participants