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: updating data sources and resources for fabric cloud router #531

Merged
merged 15 commits into from
Feb 13, 2024

Conversation

srushti-patl
Copy link
Contributor

@srushti-patl srushti-patl commented Jan 19, 2024

  • Moved fabric_cloud_Router_read schema to fabric_cloud_router data_source.go
  • Moved fabric_cloud_router_schema to fabric_cloud_router_resource.go
  • Moved Cloud router dependent methods from fabric_mapping_helpers.go to fabric_cloud_router_resource.go
  • Updated documentation for fabric cloud router data sources & resources

- `equinix_asn` (Number) Equinix ASN
- `href` (String) Fabric Cloud Router URI information
- `id` (String) The ID of this resource.
- `location` (Set of Object) Fabric Cloud Router location (see [below for nested schema](#nestedatt--location))
- `name` (String) Fabric Cloud Router name. An alpha-numeric 24 characters string which can include only hyphens and underscores
- `notifications` (List of Object) Preferences for notifications on Fabric Cloud Router configuration or status changes (see [below for nested schema](#nestedatt--notifications))
- `order` (Set of Object) Order information related to this Fabric Cloud Router (see [below for nested schema](#nestedatt--order))
- `package` (Set of Object) Fabric Cloud Router package information (see [below for nested schema](#nestedatt--package))
- `project` (Block Set) Project information (see [below for nested schema](#nestedblock--project))
- `package` (Set of Object) Fabric Cloud Router location (see [below for nested schema](#nestedatt--package))
Copy link
Contributor

Choose a reason for hiding this comment

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

The description of this seems to be incorrect. Should refer to package.

@thogarty
Copy link
Contributor

References #423

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.

Some changes requested. Good start.

docs/data-sources/equinix_fabric_cloud_router.md Outdated Show resolved Hide resolved
@srushti-patl
Copy link
Contributor Author

I have tested above changes with cloud-router-2-aws example script:
Screenshot 2024-01-19 at 5 06 42 PM
Screenshot 2024-01-19 at 5 07 05 PM

@thogarty thogarty changed the title fix: updating data sources and resources for fabric cloud router refactor: updating data sources and resources for fabric cloud router Jan 23, 2024
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.

Some Changes Requested.

internal/schema/fabric_common_mapping_helpers.go Outdated Show resolved Hide resolved
internal/schema/fabric_common_mapping_helpers.go Outdated Show resolved Hide resolved
internal/schema/fabric_common_schemas.go Outdated Show resolved Hide resolved
@srushti-patl srushti-patl requested a review from a team as a code owner February 6, 2024 00:23
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.

Some minor changes to schema methods and descriptions requested. Please update docs after changes are made. Thank you!

)

func resourceCloudRouter() *schema.Resource {
func FabricCloudRouterPackageSch() map[string]*schema.Schema {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can lower case the first letter on this so that it's not exported from the equinix package. It becomes a breaking change when the interface changes like that. All Uppercase Methods/Structs/Variables will be exported automatically.

},
}
}
func FabricCloudRouterAccountSch() map[string]*schema.Schema {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lowercase method names because we don't need to export them. As shown in first comment.

},
}
}
func FabricCloudRouterProjectSch() map[string]*schema.Schema {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lowercase method names because we don't need to export them. As shown in first comment.

}
}

func FabricCloudRouterResourceSchema() map[string]*schema.Schema {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lowercase method names because we don't need to export them. As shown in first comment.

Type: schema.TypeString,
Required: true,
ValidateFunc: validation.StringInSlice([]string{"XF_ROUTER"}, true),
Description: "Defines the FCR type like XF_ROUTER",
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be updated to a more professional description. Defines the FCR type; i.e. XF_ROUTER

"project": {
Type: schema.TypeSet,
Required: true,
Description: "Customer resource hierarchy project information.\nApplicable to customers onboarded to Equinix Identity and Access Management. For more information see Identity and Access Management: Projects",
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove the newline \n here. It's ok to have single line descriptions.

"bgp_ipv4_routes_count": {
Type: schema.TypeInt,
Computed: true,
Description: "Access point used and maximum number of IPv4 BGP routes",
Copy link
Contributor

Choose a reason for hiding this comment

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

This Should just be Maximum number of IPv4 BGP Routes. Though I suspect that it's not a maximum and is just the total number of routes currently in use. Double check with: https://developer.equinix.com/docs?page=/dev-docs/fabric/overview

"bgp_ipv6_routes_count": {
Type: schema.TypeInt,
Computed: true,
Description: "Access point used and maximum number of IPv6 BGP routes",
Copy link
Contributor

Choose a reason for hiding this comment

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

This Should just be Maximum number of IPv6 BGP Routes. Though I suspect that it's not a maximum and is just the total number of routes currently in use. Double check with: https://developer.equinix.com/docs?page=/dev-docs/fabric/overview

"distinct_ipv4_prefixes_count": {
Type: schema.TypeInt,
Computed: true,
Description: "Number of IPv4 BGP routes in use (including non-distinct prefixes).",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this description should be the above, and it seems they are switched. Look in the documentation link shared and update.

"connections_count": {
Type: schema.TypeInt,
Computed: true,
Description: "Number of connections associated with this Access point",
Copy link
Contributor

Choose a reason for hiding this comment

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

There isn't any access point referred to here. We should label it as just a cloud router, but double check with documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's Total number of connections to the given Fabric Cloud Router instance. I have updated it in the schema description.

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.

Ready to ship.

@thogarty thogarty added the area/resources/fabric Issues related to Fabric and ECX APIs label Feb 13, 2024
@srushti-patl srushti-patl merged commit feb1325 into main Feb 13, 2024
6 of 9 checks passed
@srushti-patl srushti-patl deleted the CXF-83327 branch February 13, 2024 21:32
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.

4 participants