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

Merge GetUnusedDeployments and GetUnusedDeploymentsJSON #52

Closed
linrl3 opened this issue Sep 3, 2023 · 5 comments · Fixed by #108
Closed

Merge GetUnusedDeployments and GetUnusedDeploymentsJSON #52

linrl3 opened this issue Sep 3, 2023 · 5 comments · Fixed by #108
Assignees
Labels
enhancement New feature or request

Comments

@linrl3
Copy link
Contributor

linrl3 commented Sep 3, 2023

I think we should merge GetUnusedDeployments(includeExcludeLists IncludeExcludeLists, kubeconfig string) and GetUnusedDeploymentsJSON(includeExcludeLists IncludeExcludeLists, kubeconfig string) (string, error) into something like GetUnusedDeployments(includeExcludeLists IncludeExcludeLists, kubeconfig string, format string).

Because they have many duplicated codes and provide similar functionality, the only difference is the output format. I see another issue is talking about adding yaml output format. At that time we may have 3 similar functions. The case also apply on other resources function GetUnusedXXX.

@yonahd
Copy link
Owner

yonahd commented Sep 3, 2023

Hi @linrl3
The Yaml and Json will be one function.
If you can come up with a good proposal to combine the tabular and Json functions nicely let's do it

@loaynaser3

This comment was marked as off-topic.

@yonahd yonahd added the enhancement New feature or request label Sep 20, 2023
@loaynaser3
Copy link
Contributor

loaynaser3 commented Sep 26, 2023

back to this here's an example form configmap supporting all output format in one function

func GetUnusedConfigmapsStructured(includeExcludeLists IncludeExcludeLists, clientset kubernetes.Interface, outputFormat string) (string, error) {
	namespaces := SetNamespaceList(includeExcludeLists, clientset)
	response := make(map[string]map[string][]string)
	for _, namespace := range namespaces {
		diff, err := processNamespaceCM(clientset, namespace)
		if err != nil {
			fmt.Fprintf(os.Stderr, "Failed to process namespace %s: %v\n", namespace, err)
			continue
		}
		if outputFormat == "" || outputFormat == "table" {
			output := FormatOutput(namespace, diff, "Config Maps")
			fmt.Println(output)
			fmt.Println()
		} else {
			resourceMap := make(map[string][]string)
			resourceMap["ConfigMap"] = diff
			response[namespace] = resourceMap
		}
	}
	jsonResponse, err := json.MarshalIndent(response, "", "  ")
	if err != nil {
		return "", err
	}

	if outputFormat == "yaml" {
		yamlResponse, err := yaml.JSONToYAML(jsonResponse)
		if err != nil {
			fmt.Printf("err: %v\n", err)
		}
		return string(yamlResponse), nil
	} else if outputFormat == "json" {
		return string(jsonResponse), nil
	} else {
		return "", nil
	}

}

and function call is

if response, err := kor.GetUnusedConfigmapsStructured(includeExcludeLists, clientset, outputFormat); err != nil {
			fmt.Println(err)
		} else {
			fmt.Println(response)
		}

@yonahd
Copy link
Owner

yonahd commented Sep 26, 2023

@loaynaser3 Sounds reasonable.
Let's wait for #77 to complete so we can see if this would still improve or the function would be overloaded

@yonahd
Copy link
Owner

yonahd commented Sep 27, 2023

@loaynaser3 #77 was merged

@yonahd yonahd linked a pull request Oct 18, 2023 that will close this issue
@yonahd yonahd self-assigned this Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants