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

fix/kubernetes-pod-status #216

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Nikhil-Singhal-06
Copy link
Member

Description

Pod status action fails to detect the pod status.

Pull request checklist

  • [] Issue created that explains the change and why it's needed
  • Tests are part of the PR (for bug fixes / features)
  • Docs reviewed and added / updated if needed (for bug fixes / features)
  • Format and Lint checks (make check) pass locally
  • PR applies Apache 2.0 License to all code files

Pull request type

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation changes
  • Other (please describe)

Current behavior

New behavior

How to test

@Nikhil-Singhal-06 Nikhil-Singhal-06 changed the title fix-kubernetes-pod-status fix/kubernetes-pod-status Oct 18, 2024
Copy link

github-actions bot commented Oct 18, 2024

Test Results

360 tests   352 ✅  38m 45s ⏱️
 17 suites    8 💤
 17 files      0 ❌

Results for commit 9f75936.

♻️ This comment has been updated with latest results.

fmirus
fmirus previously approved these changes Oct 22, 2024
if not isinstance(status, tuple) or not isinstance(status[0], str):
raise ValueError("Status expected to be enum.")
self.expected_status = status[0]
self.regex = regex
self.current_state = KubernetesWaitForPodStatusState.MONITORING
self.is_pod = False
self.monitoring_thread = threading.Thread(target=self.watch_pods, daemon=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

this creates a new thread for every execution. Instead please rework the previous solution
--> in update() when getting the last element from update_queue (line 58) , store the item in a member variable (e.g. self.current_state). This allows you to keep the state for another execution. Then you can rework the update() function:

  1. move the check within update() into a separate function.
  2. update() should look something like that afterwards:
if self.update_queue().empty():
  if self.current_state is not None and check(self.current_state):
    return  SUCCESS
  return RUNNING
else:
  while not self.update_queue.empty():
    self.current_state = self.update_queue.get()
    if check(self.current_state):
      return SUCCESS
  return RUNNING

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.

3 participants