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

Delete application profiles based on template hash #78

Merged
merged 1 commit into from
Dec 24, 2023

Conversation

dwertent
Copy link

@dwertent dwertent commented Dec 24, 2023

Type

bug_fix


Description

  • The PR primarily focuses on modifying the deletion strategy of application profiles based on template hash.
  • The deleteByWlid function has been replaced by deleteByTemplateHashOrWlid for "applicationactivities", "applicationprofiles", and "applicationprofilesummaries" in the resourceKindToHandler map.
  • A new function deleteByTemplateHashOrWlid has been added which deletes based on template hash or wlid.
  • The function fetchInstanceIdsAndImageIdsFromRunningPods has been renamed to fetchInstanceIdsAndImageIdsAndReplicasFromRunningPods and now also fetches replicas from running pods.
  • A new field RunningTemplateHash has been added to the ResourceMaps struct.
  • The version of the dependency github.com/kubescape/k8s-interface has been updated from v0.0.153 to v0.0.154.

PR changes walkthrough

Relevant files                                                                                                                                 
Bug fix
1 files
cleanup.go                                                                                                   
    pkg/cleanup/cleanup.go

    This file has seen changes in the import statements and the
    resourceKindToHandler map. The function deleteByWlid has
    been replaced by deleteByTemplateHashOrWlid for the keys
    "applicationactivities", "applicationprofiles", and
    "applicationprofilesummaries". A new function
    deleteByTemplateHashOrWlid has been added which deletes
    based on template hash or wlid.

+19/-7
Enhancement
1 files
discovery.go                                                                                               
    pkg/cleanup/discovery.go

    The function fetchInstanceIdsAndImageIdsFromRunningPods
    has been renamed to
    fetchInstanceIdsAndImageIdsAndReplicasFromRunningPods and
    now also fetches replicas from running pods. A new field
    RunningTemplateHash has been added to the ResourceMaps
    struct.

+9/-2
Dependencies
2 files
go.mod                                                                                                           
    go.mod

    The version of the dependency
    github.com/kubescape/k8s-interface has been updated from
    v0.0.153 to v0.0.154.

+1/-1
go.sum                                                                                                           
    go.sum

    The checksums for the dependency
    github.com/kubescape/k8s-interface have been updated to
    reflect the new version v0.0.154.

+2/-2

User description

Sorry, we do not accept changes directly against this repository. Please see
CONTRIBUTING.md for information on where and how to contribute instead.

Copy link

PR Description updated to latest commit (8b92cc2)

Copy link

PR Analysis

  • 🎯 Main theme: Modifying the deletion strategy of application profiles based on template hash
  • 📝 PR summary: This PR introduces changes to the deletion strategy of application profiles in the cleanup process. It replaces the deleteByWlid function with deleteByTemplateHashOrWlid for certain resources. A new field RunningTemplateHash is added to the ResourceMaps struct. The function fetchInstanceIdsAndImageIdsFromRunningPods is renamed and now also fetches replicas from running pods. The version of the dependency github.com/kubescape/k8s-interface is updated.
  • 📌 Type of PR: Bug fix
  • 🧪 Relevant tests added: No
  • ⏱️ Estimated effort to review [1-5]: 3, because the PR involves changes in the deletion strategy which requires careful review to ensure it doesn't introduce any issues. The PR is of moderate size and complexity.
  • 🔒 Security concerns: No security concerns found

PR Feedback

  • 💡 General suggestions: It would be beneficial to include unit tests for the new function deleteByTemplateHashOrWlid to ensure its correctness. Also, consider adding comments to the new and updated functions to provide context and explain their purpose and how they work.

  • 🤖 Code feedback:
    relevant filepkg/cleanup/cleanup.go
    suggestion      

    Consider handling the case where both wlReplica and wlid are not found in the deleteByTemplateHashOrWlid function. Currently, if both are not found, the function falls back to deleteByWlid which might not be the expected behavior. [important]

    relevant linefunc deleteByTemplateHashOrWlid(_, _ string, metadata *metav1.ObjectMeta, resourceMaps ResourceMaps) bool {

    relevant filepkg/cleanup/discovery.go
    suggestion      

    It would be good to handle the case where the pod-template-hash label is not found in the fetchInstanceIdsAndImageIdsAndReplicasFromRunningPods function. Currently, if the label is not found, it is ignored which might not be the expected behavior. [medium]

    relevant lineif replicaHash, ok := pod.GetLabel("pod-template-hash"); ok {

    How to use

    Instructions

    To invoke the PR-Agent, add a comment using one of the following commands:
    /review: Request a review of your Pull Request.
    /describe: Update the PR title and description based on the contents of the PR.
    /improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback.
    /ask <QUESTION>: Ask a question about the PR.
    /update_changelog: Update the changelog based on the PR's contents.
    /add_docs: Generate docstring for new components introduced in the PR.
    /generate_labels: Generate labels for the PR based on the PR's contents.
    see the tools guide for more details.

    To edit any configuration parameter from the configuration.toml, add --config_path=new_value.
    For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
    To list the possible configuration parameters, add a /config comment.

Copy link

Summary:

  • License scan: failure
  • Credentials scan: success
  • Vulnerabilities scan: failure
  • Unit test: success
  • Go linting: success

@dwertent dwertent merged commit 1583538 into main Dec 24, 2023
6 checks passed
@matthyx matthyx deleted the fix-delete-applicationprofiles-objects branch October 1, 2024 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant