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

Added input to ibm container data source in order to skip listing bounded services #2051

Merged
merged 1 commit into from
Nov 23, 2020

Conversation

jfeddern
Copy link

Hi there,
a lot of services are bound to our Kubernetes cluster. Everytime we run the Terraform scripts the ibm_container_cluster data source is executed which fetches all bounded services. In our case this leads to a very long execution time as the API does not respond very fast when there are more than 100 services bound to the cluster (actually it even results in timeout exceptions).

As we do not even need the information about all bounded services I would like to introduce the possibility to add an additional variable to filter for specific namespaces.

This should improve the overall performance of the data source and also the reliability.
The default value still remains as an empty string, so the changes should be backwards compatible.

In case of any questions, don't hesitate to contact me.

@jfeddern jfeddern force-pushed the fetch-not-all-bounded-services branch from 607555e to 645cf11 Compare November 20, 2020 06:56
"bounded_services_namespace_id": {
Description: "Name of the Kubernetes namespace where the service is bounded to.",
Type: schema.TypeString,
Default: "",
Copy link
Collaborator

Choose a reason for hiding this comment

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

If its optional then by default value will be ""..
Remove Default=""

@@ -352,7 +359,8 @@ func dataSourceIBMContainerClusterRead(d *schema.ResourceData, meta interface{})
for i, worker := range workerFields {
workers[i] = worker.ID
}
servicesBoundToCluster, err := csAPI.ListServicesBoundToCluster(name, "", targetEnv)
boundedServicesNamespaceID := d.Get("bounded_services_namespace_id").(string)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since its an optional fields use d.GetOk

var namespaceID string
if nID, ok := d.GetOk("bounded_services_namespace_id"); ok {
namespaceID = nID.(string)
}
	servicesBoundToCluster, err := csAPI.ListServicesBoundToCluster(name, namespaceID, targetEnv)
	if err != nil {
		return fmt.Errorf("Error retrieving services bound to cluster: %s", err)
	}

@hkantare
Copy link
Collaborator

@jfeddern jfeddern force-pushed the fetch-not-all-bounded-services branch 2 times, most recently from 4fbf2b0 to e8fdb47 Compare November 20, 2020 07:22
@jfeddern jfeddern changed the title Added bounded service namespace id as input to ibm container data source Added input to ibm container data source in order to skip listing bounded services Nov 20, 2020
@jfeddern jfeddern force-pushed the fetch-not-all-bounded-services branch from e8fdb47 to d2c52e4 Compare November 20, 2020 07:29
@jfeddern
Copy link
Author

Hi @hkantare ,
thanks for your feedback.
In the meantime I was thinking about a better solution. As already mentioned in our case its not even needed to fetch any bounded services. Therefore I prefer to introduce a flag as an input field. If the flag is set to true, we won't even use the API in this case.
This would also reduce the overall load for the API endpoint itself. What do you think, does this solution makes more sense?

Kind regards,
Jan

@hkantare
Copy link
Collaborator

Hi
Yes flag should be best to control the behaviour. Name the attribute as
list_bounded_services by default set to true..If flag is set to false you can skip the call to the API...

@jfeddern jfeddern force-pushed the fetch-not-all-bounded-services branch from d2c52e4 to 3703100 Compare November 20, 2020 07:48
@jfeddern
Copy link
Author

Alright, thank you.
I updated the name of the flag, the default value and also the documentation.

@hkantare hkantare merged commit f7b3b0e into IBM-Cloud:master Nov 23, 2020
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.

2 participants