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

feat: Adding fabric_network_schema_resource #553

Merged
merged 48 commits into from
Feb 13, 2024
Merged

Conversation

srushti-patl
Copy link
Contributor

@srushti-patl srushti-patl commented Feb 1, 2024

Includes:
fabric_network_data_source
fabric_network_resource
fabric_network_data_source_acc_test
fabric_network_resource_acc_test

@srushti-patl srushti-patl requested review from a team as code owners February 1, 2024 07:31
@srushti-patl srushti-patl changed the title Feat: Adding fabric_network_schema_resource feat: Adding fabric_network_schema_resource Feb 1, 2024
@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2024

Codecov Report

Attention: 610 lines in your changes are missing coverage. Please review.

Comparison is base (7048527) 57.27% compared to head (fcfa4e3) 39.53%.
Report is 2 commits behind head on main.

Files Patch % Lines
equinix/resource_fabric_service_profile.go 47.87% 393 Missing ⚠️
equinix/resource_fabric_network.go 40.62% 209 Missing ⚠️
equinix/data_source_fabric_network.go 84.00% 4 Missing ⚠️
equinix/fabric_mapping_helper.go 0.00% 3 Missing ⚠️
equinix/resource_fabric_connection.go 50.00% 1 Missing ⚠️

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

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #553       +/-   ##
===========================================
- Coverage   57.27%   39.53%   -17.75%     
===========================================
  Files         100       98        -2     
  Lines       18963    18692      -271     
===========================================
- Hits        10861     7389     -3472     
- Misses       7776    11258     +3482     
+ Partials      326       45      -281     

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

Copy link
Contributor

@thogarty thogarty left a comment

Choose a reason for hiding this comment

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

Changes requested.

equinix/resource_fabric_network.go Outdated Show resolved Hide resolved
equinix/resource_fabric_network.go Outdated Show resolved Hide resolved
equinix/resource_fabric_network.go Outdated Show resolved Hide resolved
equinix/resource_fabric_network.go Outdated Show resolved Hide resolved
Copy link
Contributor

@thogarty thogarty left a comment

Choose a reason for hiding this comment

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

Still have some changes requested. I added exactly what the changes I'm looking for are so you can just replace them.

fabricNetwork, _, err := client.NetworksApi.GetNetworkByUuid(ctx, d.Id())
if err != nil {
diag.Errorf("[WARN] Fabric Network %s not found , error %s", d.Id(), equinix_errors.FormatFabricError(err))
return diag.FromErr(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

We still have this update that needs to be made. I'm looking for line 211 to be removed; and for line 212 to be return diag.FromErr(equinix_errors.FormatFabricError(err))

updates := []v4.NetworkChangeOperation{update}
_, res, err := client.NetworksApi.UpdateNetworkByUuid(ctx, updates, d.Id())
if err != nil {
return diag.Errorf("error response for the Fabric Network update, response %v, error %v", res, equinix_errors.FormatFabricError(err))
Copy link
Contributor

Choose a reason for hiding this comment

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

This line should just be return diag.FromErr(equinix_errors.FormatFabricError(err))

}
update, err := getFabricNetworkUpdateRequest(dbConn, d)
if err != nil {
return diag.FromErr(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

This line should be return diag.Errorf("error retrieving intended updates from network config: %v", err)

return diags
}
}
return diag.Errorf("error response for the Fabric Network delete. Error %v and response %v", equinix_errors.FormatFabricError(err), resp)
Copy link
Contributor

Choose a reason for hiding this comment

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

This line should just be return diag.FromErr(equinix_errors.FormatFabricError(err))

Copy link
Contributor

@thogarty thogarty left a comment

Choose a reason for hiding this comment

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

LGTM. Ready for review from DevRel.

@thogarty thogarty added the area/resources/fabric Issues related to Fabric and ECX APIs label Feb 13, 2024
@srushti-patl srushti-patl merged commit df3c52d into main Feb 13, 2024
7 of 9 checks passed
@srushti-patl srushti-patl deleted the CXF-83378-update branch February 13, 2024 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/resources/fabric Issues related to Fabric and ECX APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants