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

NFV-18043: modify cluster fields and add mgmt acl in existing device API #9

Merged
merged 4 commits into from
Mar 15, 2022

Conversation

rling-equinix
Copy link
Contributor

@rling-equinix rling-equinix commented Mar 11, 2022

This PR changes the format of ClusterDetails and ClusterNodeDetail. ClusterNode is no longer needed in client.go because we will introduce Node0 and Node1 in ClusterDetails while also removing the previous container of these fields, ClusterNodeDetails.

This simplifies the structures and keeps client.go consistent with the Terraform changes being introduced in equinix/terraform-provider-equinix#105.

@displague
Copy link
Member

This simplifies the structures and keeps client.go consistent with the Terraform changes

I think we don't need to uphold consistency in this client with the interface provided in Terraform. We can continue the discussion about what we expect in our go types in #11.

Other tools may depend on this interface and we don't want to introduce breaking changes each time we want to change Terraform, which operates under a different pattern for versioning.

I've created #10 to address versioning in the future.

Per our conversation, let's keep the existing fields and types and perhaps introduce the new fields in the existing types (or new types).

@rling-equinix
Copy link
Contributor Author

NOTE: Scope of this library is limited to needs of Terraform provider plugin and it is not providing full capabilities of Equinix Network Edge API

The purpose of ne-go repository is for terraform provider development. We introduce client.go along with those types to further combine request and response class into one object so that other developers can better maintain it in our provider side code.

The ne-go changes have to come first before any PRs sent for review in terraform provider repo. We have to find out a better way for this version management because it's common that we might make breaking changes in ne-go based on our discussion and need for the provider side changes. At that moment, PR is not merged at provider side but ne-go has been published. We face the dilemma where we cannot change ne-go because of the breaking change. However, we are the ones who introduce them in the first place and no one has consumed those changes at that time. There should be a way to publish temporary version for ne-go and after code changes are merged in provider side, we could mark this temporary version a final one.

@rling-equinix
Copy link
Contributor Author

For this PR, those fields that are not required will be marked as deprecated to follow the versioning strategy here.

Copy link
Contributor

@ocobles ocobles left a comment

Choose a reason for hiding this comment

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

I've left 1 comment. Everything else looks fine to me

rest_device.go Outdated
@@ -307,11 +315,12 @@ func mapDeviceClusterDetailsDomainToAPI(clusterDetails *ClusterDetails) *api.Clu
}
req := api.ClusterDetailsRequest{}
req.ClusterName = clusterDetails.ClusterName
req.NumOfNodes = clusterDetails.NumOfNodes
clusterNodeDetailsRequest := make(map[string]api.ClusterNodeDetailRequest)
for k, v := range clusterDetails.ClusterNodeDetails {
Copy link
Contributor

Choose a reason for hiding this comment

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

This for loop is no longer required since clusterNodeDetailsRequest["node0"] and clusterNodeDetailsRequest["node1"] is being defined below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@displague displague merged commit 826e3e4 into equinix:master Mar 15, 2022
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