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

Add Pod Status when pod is pending by going through pending container #2932

Merged
merged 4 commits into from
Oct 9, 2019

Conversation

tejal29
Copy link
Contributor

@tejal29 tejal29 commented Sep 24, 2019

Relates to #176

Add Pod Status when pod is pending by going through pending container.

Description

User facing changes

n/a

Next PRs.

Use this #2928

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes unit tests
  • n/a Mentions any output changes.
  • n/a Adds documentation as needed: user docs, YAML reference, CLI reference.
  • n/a Adds integration tests if needed.

Reviewer Notes

  • The code flow looks good.
  • Unit test added.
  • User facing changes look good.

Release Notes

@codecov
Copy link

codecov bot commented Sep 24, 2019

Codecov Report

Merging #2932 into master will increase coverage by 0.03%.
The diff coverage is 77.77%.

Impacted Files Coverage Δ
pkg/skaffold/kubernetes/status_check.go 77.77% <77.77%> (ø)

}

func GetPodDetails(client kubernetes.Interface, ns string, podName string) PodStatus {
pi := client.CoreV1().Pods(ns)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pi := client.CoreV1().Pods(ns)
pods := client.CoreV1().Pods(ns)

Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

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

This looks good to me.
I think we should add the same logic though for EphemeralContainerStatuses and InitContainerStatuses by appending them to the ContainerStatuses.

@balopat balopat changed the title Add Pod Stauts when pod is pending by going through pending container Add Pod Status when pod is pending by going through pending container Oct 4, 2019
@tejal29
Copy link
Contributor Author

tejal29 commented Oct 7, 2019

@balopat please take a look again!

pkg/skaffold/kubernetes/status_check.go Outdated Show resolved Hide resolved
pkg/skaffold/kubernetes/status_check.go Outdated Show resolved Hide resolved
default:
reason, detail := getWaitingContainerStatus(pod.Status.ContainerStatuses)
return newPendingStatus(reason, detail)

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove newline

}

func (e *PodErr) Error() string {
return fmt.Sprintf("pod in error due to reason: %s, message: %s", e.reason, e.message)
Copy link
Contributor

Choose a reason for hiding this comment

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

this error message is a bit confusing to me. what is the reason vs the message? like shouldn't the message implicitly have the reason that the pod is failing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reason is the error code and message is Message explaining Reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

an example would be "Reasan: ErrImgPull", Message: "image:tag could not be fetched"
Just Reason is not enough.

pkg/skaffold/kubernetes/status_check.go Outdated Show resolved Hide resolved
@tejal29 tejal29 merged commit b747758 into GoogleContainerTools:master Oct 9, 2019
@tejal29 tejal29 deleted the add_pod_test branch April 15, 2021 07:35
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.

4 participants