delete devops namespace when deleting devops project#1070
delete devops namespace when deleting devops project#1070stoneshi-yunify merged 1 commit intokubesphere:masterfrom
Conversation
Signed-off-by: renyunkang <rykren1998@gmail.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a bug where namespaces linked to a DevOps project were not being cleaned up upon the project's deletion. It introduces a mechanism within the DevOps project controller to identify and remove these associated namespaces, improving resource management and ensuring proper cleanup when a project is deleted. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds logic to delete associated namespaces when a DevOpsProject is deleted. The implementation correctly uses a finalizer to ensure cleanup and handles errors gracefully during namespace deletion. My feedback focuses on improving logging to use structured logging consistently and clarifying a code comment, which will enhance observability and maintainability.
| // find all namespaces with the prefix of devopsproject | ||
| namespaces, err := c.namespaceLister.List(labels.SelectorFromSet(labels.Set{constants.DevOpsProjectLabelKey: project.Name})) | ||
| if err != nil { | ||
| klog.ErrorS(err, fmt.Sprintf("failed to list namespaces when deleting devopsproject %s", key)) | ||
| return err | ||
| } | ||
| for _, ns := range namespaces { | ||
| klog.Infof("deleting namespace %s for devopsproject %s", ns.Name, project.Name) | ||
| err := c.client.CoreV1().Namespaces().Delete(context.Background(), ns.Name, metav1.DeleteOptions{}) | ||
| if err != nil && !errors.IsNotFound(err) { | ||
| // do not return error, continue to delete other namespaces | ||
| klog.ErrorS(err, fmt.Sprintf("failed to delete namespace %s for devopsproject %s", ns.Name, project.Name)) | ||
| } | ||
| } |
There was a problem hiding this comment.
The logging within this block could be improved by consistently using structured logging (klog.InfoS and klog.ErrorS) instead of klog.Infof and fmt.Sprintf. This enhances readability and makes logs easier to parse and query for observability purposes. Additionally, the leading comment can be improved to better describe the purpose of this code block.
| // find all namespaces with the prefix of devopsproject | |
| namespaces, err := c.namespaceLister.List(labels.SelectorFromSet(labels.Set{constants.DevOpsProjectLabelKey: project.Name})) | |
| if err != nil { | |
| klog.ErrorS(err, fmt.Sprintf("failed to list namespaces when deleting devopsproject %s", key)) | |
| return err | |
| } | |
| for _, ns := range namespaces { | |
| klog.Infof("deleting namespace %s for devopsproject %s", ns.Name, project.Name) | |
| err := c.client.CoreV1().Namespaces().Delete(context.Background(), ns.Name, metav1.DeleteOptions{}) | |
| if err != nil && !errors.IsNotFound(err) { | |
| // do not return error, continue to delete other namespaces | |
| klog.ErrorS(err, fmt.Sprintf("failed to delete namespace %s for devopsproject %s", ns.Name, project.Name)) | |
| } | |
| } | |
| // Clean up associated namespaces before removing the finalizer. | |
| namespaces, err := c.namespaceLister.List(labels.SelectorFromSet(labels.Set{constants.DevOpsProjectLabelKey: project.Name})) | |
| if err != nil { | |
| klog.ErrorS(err, "failed to list namespaces when deleting devopsproject", "devopsproject", key) | |
| return err | |
| } | |
| for _, ns := range namespaces { | |
| klog.InfoS("deleting namespace for devopsproject", "namespace", ns.Name, "devopsproject", project.Name) | |
| err := c.client.CoreV1().Namespaces().Delete(context.Background(), ns.Name, metav1.DeleteOptions{}) | |
| if err != nil && !errors.IsNotFound(err) { | |
| // do not return error, continue to delete other namespaces | |
| klog.ErrorS(err, "failed to delete namespace", "namespace", ns.Name, "devopsproject", project.Name) | |
| } | |
| } |
|
/lgtm |
2 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes # https://github.com/kubesphere/project/issues/7404
Special notes for reviewers:
Please check the following list before waiting reviewers:
Does this PR introduce a user-facing change??