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.

Sign up for GitHub

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

Jump to bottom

chore: equinix_metal_vlan plugin sdk v2 to framework migration #578

Merged
merged 9 commits into from
Mar 19, 2024
Merged

chore: equinix_metal_vlan plugin sdk v2 to framework migration #578

merged 9 commits into from
Mar 19, 2024

Conversation

aayushrangwala
Copy link
Contributor

@aayushrangwala aayushrangwala commented Feb 19, 2024 •
edited by displague
Loading

Part of #612

@aayushrangwala aayushrangwala requested review from t0mk and a team as code owners February 19, 2024 14:38
@displague displague changed the title chore: Vlans plugin sdk v2 to framework migration chore: equinix_metal_vlan plugin sdk v2 to framework migration Feb 20, 2024
@codecov-commenter
Copy link

codecov-commenter commented Feb 20, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 4.95283% with 403 lines in your changes are missing coverage. Please review.

Project coverage is 39.04%. Comparing base (e6d1b41) to head (dba062f).

Files Patch % Lines
internal/resources/metal/vlan/resource.go 0.00% 141 Missing ⚠️
internal/resources/metal/vlan/datasource_schema.go 0.00% 74 Missing ⚠️
internal/resources/metal/vlan/datasource.go 15.29% 70 Missing and 2 partials ⚠️
internal/resources/metal/vlan/resource_schema.go 0.00% 62 Missing ⚠️
internal/resources/metal/vlan/models.go 0.00% 39 Missing ⚠️
internal/planmodifiers/caseinsensitive.go 42.10% 9 Missing and 2 partials ⚠️
internal/resources/metal/vlan/sweeper.go 0.00% 4 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #578      +/-   ##
==========================================
- Coverage   39.89%   39.04%   -0.85%     
==========================================
  Files         109      114       +5     
  Lines       18977    19186     +209     
==========================================
- Hits         7570     7491      -79     
- Misses      11202    11489     +287     
- Partials      205      206       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@displague displague mentioned this pull request Feb 22, 2024
Closed
@ctreatma
Copy link
Contributor

There are still a bunch of tests failing due to how the provider is set up. I updated all Metal acceptance tests in #586. Now that that is merged, you can rebase this PR on main and that should reduce the number of test failures.

@aayushrangwala aayushrangwala requested a deployment to external February 23, 2024 20:40 — with GitHub Actions Abandoned
@aayushrangwala aayushrangwala requested a deployment to external March 18, 2024 15:40 — with GitHub Actions Abandoned
@aayushrangwala aayushrangwala requested a deployment to external March 19, 2024 14:06 — with GitHub Actions Abandoned
@ctreatma
Copy link
Contributor

ctreatma commented Mar 19, 2024 •
edited
Loading

Needed to update the resource schema slightly so that:

  • facility only triggers replacement if it's explicitly configured
  • facility doesn't default to an empty string
  • metro change always requires replacement

Commit with those changes is here: 5fc7417

ctreatma
ctreatma approved these changes Mar 19, 2024
View reviewed changes
Copy link
Contributor

@ctreatma ctreatma left a comment

Choose a reason for hiding this comment

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

After updating the resource schema to handle switching from facility to metro, the only test failure in CI was a post-test cleanup failure in a metal port test.

@ctreatma ctreatma merged commit e86dd8a into equinix:main Mar 19, 2024
4 of 5 checks passed
@displague displague mentioned this pull request Apr 5, 2024
Merged
ctreatma added a commit that referenced this pull request Apr 10, 2024
@ctreatma
fix: avoid inconsistent state errors for vlans (#642)
1e82c46 
When the equinix_metal_vlan resource was migrated (#578) from
terraform-plugin-sdk/v2 (SDKv2) to terraform-plugin-framework
(framework), some unintended behavior was introduced around the
description and metro attributes.

For equinix_metal_vlan resources under SDKv2, description was an
optional attribute and metro was case insensitive, but after migration
from SDKv2 to framework, the description was set to `""` when the
attribute was omitted, and metro was required to be lower-case to avoid
terraform errors.

This updates the VLAN model parsing function so that:
- if there is already metro value configured in state and it is equal to
the value received from the API or from the config, ignoring case, then
the value that is already in state is used instead of the value from the
API
- if there is no description in the API response, the description
property is omitted entirely instead of being set to an empty string

`TestAccMetalVlan_NoDescription` has been added to validate the resource
behavior when the description is omitted and `TestAccMetalVlan_metro`
has been updated to validate behavior when metro is specified in
uppercase.

Fixes #633
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ocobles ocobles ocobles left review comments

@displague displague displague left review comments

@ctreatma ctreatma ctreatma approved these changes

@t0mk t0mk Awaiting requested review from t0mk

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aayushrangwala @codecov-commenter @ctreatma @displague @ocobles