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

Handle StandalonePods Succeeded case when checking status #9580

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Y--
Copy link

@Y-- Y-- commented Nov 21, 2024

Description
When Skaffold checks a standalone pod's status (method checkStandalonePodsStatus) , it currently only considers Failed and Running state, any other status will lead the pod to be systematically added to the pendingPods collection.

According to the documentation, a pod can be in 5 different states: Pending, Running, Succeeded, Failed or Unknown.

While it make sense for Pending or Unknown pods to be added to the pendingPods collection, I think it does not for the Succeeded ones (pods that terminated successfully)

This PR introduces a tiny change which just skips Succeeded pods.

Tests

I tried to look at other test and could only find:

  • unit tests on CheckStatus that don't involve StandalonePods resource
  • TestPollServiceStatus, TestPollJobStatus, TestPollDeployment, none of which involve StandalonePods resource

Please let me know if I've missed it, I'd be happy to add test on it.

Thanks!

Copy link

google-cla bot commented Nov 21, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link

conventional-commit-lint-gcf bot commented Nov 21, 2024

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@mattsanta
Copy link
Contributor

Thanks for the PR!

So the current behavior is that status checking does not work if the Pod has completed and has a Succeeded phase, skaffold will continue to poll the Pod status until it times out. I also went ahead and tested your fix by building skaffold locally and it looks like it works.

I know the current tests don't ever use StandalonePods, but could you be the first one to add a test case that does for this scenario?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants