Addition of new network resource module for nd_multi_cluster_connectivity (DCNE-458)#170
Addition of new network resource module for nd_multi_cluster_connectivity (DCNE-458)#170shrsr wants to merge 3 commits intoCiscoDevNet:masterfrom
Conversation
…luster_connectivity
shrsr
requested review from
akinross,
anvitha-jain,
gmicol,
lhercot,
sajagana and
samiib
as code owners
shrsr
changed the title
| callbacks = { | ||
| "update_callback": update_cluster, | ||
| "create_callback": create_cluster, | ||
| "delete_callback": delete_cluster, | ||
| } |
There was a problem hiding this comment.
Did you consider using classes/interfaces instead of callback functions? Some sort of base state manager abstraction.
I think this way we do not have to pass in callback, but we could initialise a state manager object. This object would have its own specific implementations per module of create/update/delete or whatever we want to standardise on the manage object.
There was a problem hiding this comment.
Yes, I had a discussion with @lhercot regarding creating an interface for the CRUD and related per module functions. I would say that would be the next step if we all decide to go this route?
| desired_clusters_map = {} | ||
| for cluster_data_input in desired_clusters_from_module_input: | ||
| normalized_payload = module_input_to_api_payload(cluster_data_input) | ||
| desired_clusters_map[get_cluster_unique_key(normalized_payload)] = normalized_payload |
There was a problem hiding this comment.
so for my understanding, is this map reflecting the desired state of a unique id ( this case the onboardUrl from spec ) and then you get the existing configuration after and put it in another map with same format? Those two maps are then passed to the manage_state function which decides what to delete / add / update?
what is the purpose of the normalized_payload and why is this not done for existing? are you conforming the input to the existing configuration?
| ) | ||
|
|
||
| nd = NDModule(module) | ||
| desired_clusters_from_module_input = nd.params.get("config") |
There was a problem hiding this comment.
should we assign variables when they are only used once after in code?
| desired_clusters_map = {} | ||
| for cluster_data_input in desired_clusters_from_module_input: | ||
| normalized_payload = module_input_to_api_payload(cluster_data_input) | ||
| desired_clusters_map[get_cluster_unique_key(normalized_payload)] = normalized_payload |
There was a problem hiding this comment.
Do you need this get_cluster_unique_key(normalized_payload) when you use basically use cluster_data_input.get("cluster_hostname")?
| desired_clusters_map = {} | ||
| for cluster_data_input in desired_clusters_from_module_input: | ||
| normalized_payload = module_input_to_api_payload(cluster_data_input) | ||
| desired_clusters_map[get_cluster_unique_key(normalized_payload)] = normalized_payload |
There was a problem hiding this comment.
Should there be validations in place to avoid duplicate keys and thus overwrites of existing dict entries?
| existing_clusters_map = {} | ||
| for cluster_data_raw in existing_clusters_raw: | ||
| # normalized_payload = convert_api_response_to_payload_format(cluster_data_raw) | ||
| existing_clusters_map[get_cluster_unique_key(cluster_data_raw)] = cluster_data_raw |
There was a problem hiding this comment.
when dict comprehension is possible should we try to use that technique?
| - nm_connect_cluster_merged is changed | ||
| - nm_connect_cluster_merged.before["173.36.219.189"] == {} | ||
| - nm_connect_cluster_merged.before["173.36.219.190"] == {} | ||
| - nm_connect_cluster_merged.after["173.36.219.189"]["spec"]["clusterType"] == "APIC" |
There was a problem hiding this comment.
what is the benefit of this spec container in the output?
| return payload | ||
|
|
||
|
|
||
| # def convert_api_response_to_payload_format(api_response_cluster_data): |
There was a problem hiding this comment.
Do we still need this? If we do, then can we add additional comments as to why?
There was a problem hiding this comment.
We don't need this now but still waiting on answers from the larger group to make api response similar to input
| def create_cluster(nd): | ||
| path = "/api/v1/infra/clusters" | ||
| response = nd.proposed | ||
| if not nd.module.check_mode: | ||
| response = nd.request(path, method="POST", data=nd.proposed) | ||
| return response | ||
|
|
||
|
|
||
| def update_cluster(nd): | ||
| path = "/api/v1/infra/clusters" | ||
| fabric_name_for_api_path = nd.existing.get("spec", {}).get("name") | ||
| response = payload = nd.proposed | ||
| payload["spec"]["name"] = fabric_name_for_api_path | ||
| if not nd.module.check_mode: | ||
| response = nd.request("{0}/{1}".format(path, fabric_name_for_api_path), method="PUT", data=payload) | ||
| return response | ||
|
|
||
|
|
||
| def delete_cluster(nd): | ||
| path = "/api/v1/infra/clusters" |
There was a problem hiding this comment.
Since path is same, can we move this outside?
There was a problem hiding this comment.
If we go this route, the plan is to create an interface where all this will be taken care of
3 participants
No description provided.