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

jira_integration - added retry mechnism in case of hitting rate limit #552

Merged
merged 4 commits into from
Jan 1, 2025

Conversation

kooomix
Copy link
Contributor

@kooomix kooomix commented Jan 1, 2025

User description


PR Type

Enhancement


Description

  • Improved log message formatting in BaseK8S class

  • Split cluster state logging into multiple lines


Changes walkthrough 📝

Relevant files
Formatting
base_k8s.py
Enhance logging format for cluster state messages               

tests_scripts/kubernetes/base_k8s.py

  • Improved log formatting for cluster state messages
  • Split single-line logging into multi-line format for better
    readability
  • +9/-2     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Duplication

    The cluster state logging code is duplicated in two places. Consider extracting the common logging logic into a separate helper method to improve maintainability.

            Logger.logger.info(
                "cluster state "
                f"{result}"
            )
    
            return
        delta_t = (datetime.now() - start).total_seconds()
        time.sleep(10)
    total_pods = self.get_pods(namespace=namespace, name=name, include_terminating=False)
    non_running_pods = [i for i in total_pods if i not in running_pods]
    Logger.logger.error("wrong number of pods are running, timeout: {} seconds. non_running_pods: {}".
                        format(timeout,
                               KubectlWrapper.convert_workload_to_dict(non_running_pods, f_json=True, indent=2)))
    
    result = subprocess.run("kubectl get pods -A", timeout=300, shell=True, text=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
    result = " ".join(result.stdout.splitlines())
    Logger.logger.info(
                "cluster state "
                f"{result}"
            )

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Security
    Enhance security by avoiding shell=True in subprocess calls to prevent command injection vulnerabilities

    Running shell commands with shell=True can be dangerous as it's vulnerable to
    command injection. Use a list of arguments instead.

    tests_scripts/kubernetes/base_k8s.py [770]

    -result = subprocess.run("kubectl get pods -A", timeout=300, shell=True, text=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
    +result = subprocess.run(["kubectl", "get", "pods", "-A"], timeout=300, text=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This is a critical security improvement that prevents potential command injection vulnerabilities. Using a list of arguments instead of shell=True is a security best practice for subprocess execution.

    9
    Possible issue
    Add error handling for shell command execution to prevent silent failures

    Add error handling for the subprocess.run() call to handle potential command
    execution failures. The current code assumes the command will always succeed.

    tests_scripts/kubernetes/base_k8s.py [770-771]

     result = subprocess.run("kubectl get pods -A", timeout=300, shell=True, text=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
    +if result.returncode != 0:
    +    Logger.logger.error(f"Failed to get cluster state: {result.stderr}")
    +    return
     result = " ".join(result.stdout.splitlines())
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a significant error handling gap that could lead to silent failures and hard-to-debug issues. Adding proper error handling for subprocess execution is crucial for system reliability.

    8

    Copy link

    github-actions bot commented Jan 1, 2025

    Failed to generate code suggestions for PR

    @kooomix kooomix changed the title Refactor logging for cluster state in BaseK8S class for improved read… jira_integration - added retry mechnism in case of hitting rate limit Jan 1, 2025
    @kooomix kooomix merged commit fa67fab into master Jan 1, 2025
    2 checks passed
    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.

    2 participants