Closed matthyx closed 6 months ago
Summary:
PR Description updated to latest commit (https://github.com/kubescape/storage/commit/d2d9042457e47aa3829b4b20657fda1a6b06c09e)
๐ก General suggestions: It would be beneficial to add unit tests for the new functionalities introduced in this PR. This will help ensure the correctness of the code and prevent potential regressions in the future. Also, consider using constants for string literals that are used multiple times in the code, such as "CLEANUP_INTERVAL".
relevant file | main.go |
suggestion | Consider handling the error when parsing the cleanup interval from the environment variable. If the parsing fails, it might be due to an invalid format provided by the user. In such a case, it would be helpful to inform the user about the expected format. [important] |
relevant line | intervalDuration, err := time.ParseDuration(interval) |
relevant file | pkg/cleanup/cleanup.go |
suggestion | It's good to see that the logging level for deletions has been changed to debug. However, it might be useful to also log the total number of deletions at the info level after the cleanup task is completed. This will provide a quick overview of the cleanup task's result without flooding the log with details. [medium] |
relevant line | logger.L().Debug("deleting", helpers.String("kind", resourceKind), helpers.String("namespace", metadata.Namespace), helpers.String("name", metadata.Name)) |
relevant file | pkg/cleanup/cleanup.go |
suggestion | It seems that the function `deleteByTemplateHashOrWlid` now looks for the template hash in the labels instead of the annotations. It would be helpful to update the function's comment to reflect this change. [medium] |
relevant line | wlReplica, wlReplicaFound := metadata.Labels[instanceidhandler.TemplateHashKey] // replica |
relevant file | pkg/cleanup/discovery.go |
suggestion | The list of workloads to cleanup has been hardcoded in the code. Consider moving this list to a configuration file or environment variables. This will make it easier to update the list without changing the code. [medium] |
relevant line | Workloads = sets.NewSet[string |
Summary:
Summary:
Summary:
Type
bug_fix, enhancement
Description
PR changes walkthrough
1 files
main.go
main.go
**The cleanup interval for resources is now configurable
through an environment variable. If the environment variable
is not set or cannot be parsed, a default value of 24 hours
is used. The cleanup handler is updated to use the new
configurable interval.**
1 files
cleanup.go
pkg/cleanup/cleanup.go
**The cleanup handler's logging has been improved to include
the cleanup interval when a cleanup task starts and to log
deletions at the debug level. The metadata loading function
has been updated to also load labels from the metadata file.
The function to delete resources by template hash or wlid
has been updated to look for the template hash in the labels
instead of the annotations. Some unused code has been
removed.**
1 files
discovery.go
pkg/cleanup/discovery.go
**The list of workloads to cleanup has been reduced to only
include cronjob, daemonset, deployment, job, pod,
replicaset, and statefulset. The functions to fetch wlids
from running workloads and to fetch instance IDs and image
IDs and replicas from running pods have been updated to
improve logging and remove unused code.**