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

Change final status check error message to be more concise. #2930

Merged

Conversation

tejal29
Copy link
Contributor

@tejal29 tejal29 commented Sep 24, 2019

Relates to #176

Description

In case status check phase fails due to one or more failed deployments, print summary of how many deployments or pods failed instead of printing the error for each failed resource.

there are 2 advantages

  • make output concise
  • removes duplication of error messages are the individual resource failed message is printed above.

Inspired by @etanshaul suggestion on internal docs.

User facing changes

yes. output changes.

Before
on master when deployments could not be stabilized,

...
Waiting for deployments to stabilize
 - default:deployment/leeroy-app Waiting for rollout to finish: 5 out of 10 new replicas have been updated...
 - default:deployment/leeroy-web Waiting for rollout to finish: 1 old replicas are pending termination...
 - default:deployment/leeroy-app failed. [1/2 deployment(s) still pending] Error: kubectl rollout status command killed.
 - default:deployment/leeroy-web failed. Error: kubectl rollout status command killed.
FATA[0062] following resources are not stable:
resource default:deployment/leeroy-app failed due to kubectl rollout status command killed
resource default:deployment/leeroy-web failed due to kubectl rollout status command killed 
tejaldesai@(master)$

After

on this PR, if deployments are stabilized,

Waiting for deployments to stabilize
 - default:deployment/leeroy-app Waiting for rollout to finish: 5 out of 10 new replicas have been updated...
 - default:deployment/leeroy-web Waiting for rollout to finish: 1 old replicas are pending termination...
 - default:deployment/leeroy-web failed. [1/2 deployment(s) still pending] Error: context deadline exceeded.
 - default:deployment/leeroy-app failed. Error: kubectl rollout status command killed.
FATA[0062] 2/2 deployment(s) failed                       
tejaldesai@(fix_status_check_err_msg)$ 

Next PRs.

Submitter Checklist

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

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

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

Reviewer Notes

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

Release Notes

Describe any user facing changes here so maintainer can include it in the release notes, or delete this block.

@codecov
Copy link

codecov bot commented Sep 24, 2019

Codecov Report

Merging #2930 into master will increase coverage by 0.47%.
The diff coverage is 37.5%.

Impacted Files Coverage Δ
pkg/skaffold/deploy/status_check.go 53.77% <37.5%> (-2.71%) ⬇️
pkg/skaffold/schema/v1beta10/config.go 100% <0%> (ø) ⬆️
pkg/skaffold/schema/v1alpha5/config.go 100% <0%> (ø) ⬆️
pkg/skaffold/schema/v1alpha1/config.go 100% <0%> (ø) ⬆️
pkg/skaffold/schema/v1beta1/config.go 100% <0%> (ø) ⬆️
pkg/skaffold/schema/v1beta7/config.go 100% <0%> (ø) ⬆️
pkg/skaffold/schema/v1beta6/config.go 100% <0%> (ø) ⬆️
pkg/skaffold/schema/v1alpha3/config.go 100% <0%> (ø) ⬆️
pkg/skaffold/schema/latest/config.go 100% <0%> (ø) ⬆️
pkg/skaffold/schema/v1beta5/config.go 100% <0%> (ø) ⬆️
... and 14 more

Copy link
Contributor

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

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

lgtm

@tejal29 tejal29 merged commit 8adf254 into GoogleContainerTools:master Sep 26, 2019
@tejal29 tejal29 deleted the fix_status_check_err_msg 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.

3 participants