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

add --exclude-namespace #30

Merged
merged 3 commits into from
Sep 3, 2023
Merged

Conversation

linrl3
Copy link
Contributor

@linrl3 linrl3 commented Aug 22, 2023

To solve #29

@linrl3
Copy link
Contributor Author

linrl3 commented Aug 22, 2023

@yonahd Take a look when you have time. Thanks!

@yonahd
Copy link
Owner

yonahd commented Aug 23, 2023

@yonahd Take a look when you have time. Thanks!

Hopefully over the weekend I'll get to it.

pkg/kor/kor.go Outdated Show resolved Hide resolved
pkg/kor/kor.go Outdated Show resolved Hide resolved
pkg/kor/kor.go Outdated Show resolved Hide resolved
cmd/kor/root.go Outdated Show resolved Hide resolved
@linrl3
Copy link
Contributor Author

linrl3 commented Aug 25, 2023

I will fix these in next few days.

@linrl3
Copy link
Contributor Author

linrl3 commented Aug 26, 2023

@yonahd Please review again, thanks!

Copy link
Owner

@yonahd yonahd left a comment

Choose a reason for hiding this comment

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

Looks good.
2 small comments

pkg/kor/kor.go Outdated Show resolved Hide resolved
pkg/kor/kor.go Outdated Show resolved Hide resolved
@yonahd yonahd linked an issue Aug 27, 2023 that may be closed by this pull request
Copy link
Owner

@yonahd yonahd left a comment

Choose a reason for hiding this comment

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

I suggest something like this

func SetNamespaceList(kubeClient *kubernetes.Clientset) []string {

	namespaces := make([]string, 0)

	includeNamespaceMap := make(map[string]struct{})
	excludeNamespaceMap := make(map[string]struct{})
	includeNamespaces := strings.Split(namespaceLists.IncludeListStr, ",")
	excludeNamespaces := strings.Split(namespaceLists.ExcludeListStr, ",")
	for _, ns := range excludeNamespaces {
		excludeNamespaceMap[ns] = struct{}{}
	}
	for _, ns := range includeNamespaces {
		includeNamespaceMap[ns] = struct{}{}
	}
	namespaceList, err := kubeClient.CoreV1().Namespaces().List(context.TODO(), metav1.ListOptions{})
	if err != nil {
		fmt.Fprintf(os.Stderr, "Failed to retrieve namespaces: %v\n", err)
		os.Exit(1)
	}

	if includeNamespaces[0] == "" {
		if excludeNamespaces[0] != "" {
			for _, ns := range namespaceList.Items {
				if _, exists := excludeNamespaceMap[ns.Name]; !exists {
					namespaces = append(namespaces, ns.Name)
				}
			}
			return namespaces
		}
		for _, ns := range namespaceList.Items {
			namespaces = append(namespaces, ns.Name)
		}
		return namespaces
	}

	if includeNamespaces[0] != "" {
		fmt.Printf("Exclude namespaces can't be used together with include namespaces. Ignoring exclude flag\n")
	}

	namespaceExists := false
	for includeNamespace := range includeNamespaceMap {
		namespaceExists = false
		for _, ns := range namespaceList.Items {
			if ns.Name == includeNamespace {
				namespaces = append(namespaces, ns.Name)
				namespaceExists = true
				break
			}
		}
		if !namespaceExists {
			fmt.Printf("Namespace '%s' from include list doesn't exist\n", includeNamespace)
		}
	}

	return namespaces
}

pkg/kor/kor.go Outdated Show resolved Hide resolved
pkg/kor/kor.go Outdated Show resolved Hide resolved
@yonahd
Copy link
Owner

yonahd commented Aug 30, 2023

Hey @linrl3
Will you have time to finish this over the weekend?

@linrl3
Copy link
Contributor Author

linrl3 commented Aug 30, 2023

Hey @linrl3 Will you have time to finish this over the weekend?

Yes. I've been busy in the past few days. I think I can write some code on the weekend.

@linrl3
Copy link
Contributor Author

linrl3 commented Sep 3, 2023

I think we should keep func SetNamespaceList(namespaceLists IncludeExcludeLists, kubeClient *kubernetes.Clientset) instead of func SetNamespaceList(kubeClient *kubernetes.Clientset).
The second one will use variables outside the function.

@yonahd How do you think?

@yonahd
Copy link
Owner

yonahd commented Sep 3, 2023

I think we should keep func SetNamespaceList(namespaceLists IncludeExcludeLists, kubeClient *kubernetes.Clientset) instead of func SetNamespaceList(kubeClient *kubernetes.Clientset). The second one will use variables outside the function.

@yonahd How do you think?

Yeah. It's my mistake in the declaration.

@linrl3
Copy link
Contributor Author

linrl3 commented Sep 3, 2023

Please have look when you are free @yonahd .

I push again to:

  • resolve conflicts
  • modify code on the suggestions in previous comments

I just run kor and get some output here:

 ./kor all -n ns1,ns2,default
namespace [ns1] not found
namespace [ns2] not found
Failed to get ingresses namespace default: the server could not find the requested resource
Unused Resources in Namespace: default
+---+---------------+---------------+
| # | RESOURCE TYPE | RESOURCE NAME |
+---+---------------+---------------+
+---+---------------+---------------+


 ./kor all -n ns1,ns2,default -e ns3
Exclude namespaces can't be used together with include namespaces. Ignoring --exclude-namespace(-e) flag
namespace [ns1] not found
namespace [ns2] not found
Failed to get ingresses namespace default: the server could not find the requested resource
Unused Resources in Namespace: default
+---+---------------+---------------+
| # | RESOURCE TYPE | RESOURCE NAME |
+---+---------------+---------------+
+---+---------------+---------------+


 ./kor all  -e default
Failed to get ingresses namespace kube-node-lease: the server could not find the requested resource
Unused Resources in Namespace: kube-node-lease
+---+---------------+---------------+
| # | RESOURCE TYPE | RESOURCE NAME |
+---+---------------+---------------+
+---+---------------+---------------+


Failed to get ingresses namespace kube-public: the server could not find the requested resource
Unused Resources in Namespace: kube-public
+---+---------------+---------------+
| # | RESOURCE TYPE | RESOURCE NAME |
+---+---------------+---------------+
| 1 | ConfigMap     | cluster-info  |
+---+---------------+---------------+


Failed to get ingresses namespace kube-system: the server could not find the requested resource
Unused Resources in Namespace: kube-system
+---+---------------+------------------------------------+
| # | RESOURCE TYPE |           RESOURCE NAME            |
+---+---------------+------------------------------------+
| 1 | ConfigMap     | extension-apiserver-authentication |
| 2 | ConfigMap     | kubeadm-config                     |
| 3 | ConfigMap     | kubelet-config-1.20                |
+---+---------------+------------------------------------+

Copy link
Owner

@yonahd yonahd left a comment

Choose a reason for hiding this comment

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

LGTM

@yonahd yonahd merged commit ab4e8b8 into yonahd:main Sep 3, 2023
1 check passed
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.

Add flag for namespace exception
3 participants