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

refactor: Framework migration metal connection resource and datasource #572

Merged
merged 14 commits into from
Feb 22, 2024

Conversation

ocobles
Copy link
Contributor

@ocobles ocobles commented Feb 15, 2024

Migration of equinix_metal_connection and datasource equinix_metal_connection‡

@codecov-commenter
Copy link

codecov-commenter commented Feb 15, 2024

Codecov Report

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

Project coverage is 42.08%. Comparing base (837e2c7) to head (d03eb33).

Files Patch % Lines
internal/resources/metal/connection/resource.go 0.00% 371 Missing ⚠️
internal/resources/metal/connection/models.go 0.00% 183 Missing ⚠️
...rnal/resources/metal/connection/resource_schema.go 0.00% 164 Missing ⚠️
...al/resources/metal/connection/datasource_schema.go 0.00% 93 Missing ⚠️
internal/resources/metal/connection/datasource.go 0.00% 45 Missing ⚠️
internal/errors/diags.go 0.00% 23 Missing ⚠️
internal/errors/must.go 0.00% 6 Missing ⚠️
internal/resources/metal/connection/speed.go 0.00% 6 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #572      +/-   ##
==========================================
- Coverage   44.46%   42.08%   -2.39%     
==========================================
  Files          93       98       +5     
  Lines       18164    18401     +237     
==========================================
- Hits         8077     7744     -333     
- Misses       9883    10453     +570     
  Partials      204      204              

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

@ctreatma
Copy link
Contributor

Can't tell where exactly it's coming from, but I see a large number of test failures in CI, and I see this error message as well:

    resource_metal_device_acc_test.go:225: Step 1/1 error: Error running pre-apply plan: exit status 1
        
        Error: failed to read schema for equinix_metal_project.test in registry.terraform.io/hashicorp/equinix: failed to retrieve schema from provider "registry.terraform.io/hashicorp/equinix": Error converting data source schema: The schema for the data source "equinix_metal_connection" couldn't be converted into a usable type. This is always a problem with the provider. Please report the following to the provider developer:
        
        AttributeName("service_tokens"): protocol version 5 cannot have Attributes set

In addition to the attribute mentioned in there, I also see references to hashicorp/equinix, but I don't know if that's an issue with our code or with the way terraform prints error messages.

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.

I can't claim to have gone through all 2000 lines of this change with a fine-toothed comb, but everything looks good to me and I see that the only test failures are for things that would not be impacted by these changes.

{
ExternalProviders: map[string]resource.ExternalProvider{
"equinix": {
VersionConstraint: "1.29.0", // latest version with resource defined on SDKv2
Copy link
Member

Choose a reason for hiding this comment

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

Does this version number need to be bumped going forward?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not necessarily, since it is the latest version available with the resource using sdkv2. In fact, we can remove this test once migrated, it helped validate compatibility

Signed-off-by: ocobleseqx <oscar.cobles@eu.equinix.com>
Signed-off-by: ocobleseqx <oscar.cobles@eu.equinix.com>
Signed-off-by: ocobleseqx <oscar.cobles@eu.equinix.com>
Signed-off-by: ocobleseqx <oscar.cobles@eu.equinix.com>
Signed-off-by: ocobleseqx <oscar.cobles@eu.equinix.com>
Signed-off-by: ocobleseqx <oscar.cobles@eu.equinix.com>
Signed-off-by: ocobleseqx <oscar.cobles@eu.equinix.com>
Signed-off-by: ocobleseqx <oscar.cobles@eu.equinix.com>
Signed-off-by: ocobleseqx <oscar.cobles@eu.equinix.com>
Signed-off-by: ocobleseqx <oscar.cobles@eu.equinix.com>
Signed-off-by: ocobleseqx <oscar.cobles@eu.equinix.com>
Signed-off-by: ocobleseqx <oscar.cobles@eu.equinix.com>
Signed-off-by: ocobleseqx <oscar.cobles@eu.equinix.com>
Signed-off-by: ocobleseqx <oscar.cobles@eu.equinix.com>
@ocobles
Copy link
Contributor Author

ocobles commented Feb 22, 2024

Lessons Learned and Changes Introduced:

  • Migrating to protocol v5, computed blocks are now implemented using a ListAttribute with an ElementType of types.ObjectType. This results in the loss of underlying field properties, meaning we no longer have descriptions of object fields. https://developer.hashicorp.com/terraform/plugin/framework/migrating/attributes-blocks/blocks-computed
  • When nested schemas contain editable fields, schema.ListNestedBlock should be used instead of schema.ListNestedAttribute, as the latter is only available for protocolv6.
  • Utilizing custom types enables us to obtain the ObjectType of the model using reflection.
  • Fields like "facility," which are optional, computed, and have the plan modifier "RequiresReplace," should use https://developer.hashicorp.com/terraform/plugin/framework/resources/plan-modification#usestateforunknown. Otherwise, when there is an update to any other field, this field would be marked as (known after apply), thus forcing a replace.
  • With the absence of StateFunc, logic must be added in Parse model. This will occur in all "metro" resources where we convert the value to lowercase. Refer to
    // TODO(ocobles) we were using "StateFunc: converters.ToLowerIf" for "metro" field in the sdkv2
    // version of this resource. StateFunc doesn't exist in terraform and it requires implementation
    // of bespoke logic before storing state. To ensure backward compatibility we ignore lower/upper
    // case diff for now, but we may want to require input upper case
    if !strings.EqualFold(metro.ValueString(), conn.Metro.Code) {
    *metro = types.StringValue(conn.Metro.Code)
    }
    for guidance.

@ocobles ocobles merged commit 053d04f into main Feb 22, 2024
4 of 5 checks passed
@ocobles ocobles deleted the framework-migr-metal-connection branch February 22, 2024 22:01
@ocobles ocobles restored the framework-migr-metal-connection branch March 5, 2024 18:22
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.

None yet

4 participants