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

Improve NewPodControllerForExistingPod to set podReady to true #2623

Merged
merged 2 commits into from
Jan 24, 2024

Conversation

viveksinghggits
Copy link
Contributor

@viveksinghggits viveksinghggits commented Jan 23, 2024

Change Overview

NewPodControllerForExistingPod function was supposed to return the podController for an existing pod. If this function is caleld, it would mean that the pod is already runnign and we are creating podController for that pod.
But this function didn't set podReady field of podController to true that was beign checked in some other methods (for example GetCommandExecutor) to make sure that the pod is running. And because of that those methods would see the pod to be not running and complain.
This PR fixes that by making sure that the pod is running and sets podReady to true.

Pull request type

Please check the type of change your PR introduces:

  • 🚧 Work in Progress
  • 🌈 Refactoring (no functional changes, no api changes)
  • 🐹 Trivial/Minor
  • 🐛 Bugfix
  • 🌻 Feature
  • 🗺️ Documentation
  • 🤖 Test

Issues

  • fixes #issue-number

Test Plan

Create podController for an existing pod and make sure we are able to successfully call GetCommandExecutor that relies on podReady field.

  • 💪 Manual
  • ⚡ Unit test
  • 💚 E2E

`NewPodControllerForExistingPod` function was supposed to return the
podController for an existing pod. If this function is caleld, it would mean
that the pod is already runnign and we are creating `podController` for that
pod.
But this function didn't set `podReady` field of podController to true that
was beign checked in some other methods to make sure that the pod is running.
And because of that those methods would see the pod to be not running and
complain.
This commit fixes that by making sure that the pod is running and sets
`podReady` to true.
@infraq infraq added this to In Progress in Kanister Jan 23, 2024
@e-sumin
Copy link
Contributor

e-sumin commented Jan 24, 2024

LGTM.

I'd probably add suggested note to the description.

Kanister automation moved this from In Progress to Reviewer approved Jan 24, 2024
@viveksinghggits
Copy link
Contributor Author

LGTM.

I'd probably add suggested note to the description.

I have added the note @e-sumin .
kueueing it.

@mergify mergify bot merged commit 0cebc07 into master Jan 24, 2024
14 checks passed
Kanister automation moved this from Reviewer approved to Done Jan 24, 2024
@mergify mergify bot deleted the podcontroller-fix branch January 24, 2024 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants