-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: Handle StandalonePods
Succeeded
case when checking status
#9580
fix: Handle StandalonePods
Succeeded
case when checking status
#9580
Conversation
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. |
🤖 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 -- conventional-commit-lint bot |
162cb2a
to
bb83f83
Compare
Thanks for the PR! So the current behavior is that status checking does not work if the Pod has completed and has a I know the current tests don't ever use |
0778a47
to
38245f5
Compare
38245f5
to
4e7875c
Compare
Thanks for the review @mattsanta ! I have added a few tests. Also, I'm not sure why the conventional commit linter isn't happy with me... I must be missing something big, but I think I have a subject & type for each commit message? |
StandalonePods
Succeeded
case when checking statusStandalonePods
Succeeded
case when checking status
It's referring to the title of the PR. I went ahead and added |
Thanks a lot for all your help and moving the PR quickly @mattsanta ! |
@mattsanta hey! are you aiming to release this anytime soon? :) |
Description
When Skaffold checks a standalone pod's status (method
checkStandalonePodsStatus
) , it currently only considersFailed
andRunning
state, any other status will lead the pod to be systematically added to thependingPods
collection.According to the documentation, a pod can be in 5 different states:
Pending
,Running
,Succeeded
,Failed
orUnknown
.While it make sense for
Pending
orUnknown
pods to be added to thependingPods
collection, I think it does not for theSucceeded
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:
CheckStatus
that don't involveStandalonePods
resourceTestPollServiceStatus
,TestPollJobStatus
,TestPollDeployment
, none of which involveStandalonePods
resourcePlease let me know if I've missed it, I'd be happy to add test on it.
Thanks!