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

Refactor Add proto.ActionableErr to diag.Resource and deploy.Resource.Status #4390

Merged
merged 3 commits into from
Jun 25, 2020

Conversation

tejal29
Copy link
Member

@tejal29 tejal29 commented Jun 24, 2020

In this PR, I have added the actionable error message to the diagnostics package.
For suggesting actionable errors for k8 infra errors, it would make sense to do it in the diag package.

Got rid of details in skaffold.deploy.Resource.Status to re-use proto.ActionableErr.Message.

Adding ActionableErr to daig.Resource package and propagating to deployment status in
skaffold.deploy.Resource.Status

Most of the code changes are in test. Very minor changes in source code files.

Fixes: #nnn
Related: Relevant tracking issues, for context
Merge before/after: Dependent or prerequisite PRs

Description

User facing changes (remove if N/A)

Follow-up Work (remove if N/A)

…le error proto

proto.ActionableErr

In this PR,
1. For suggesting actionable errors for k8 infra errors, it would make
sense to do it in the diag package.
2. Hence adding ActionableErr to diag.Resource package and propogating to depoyment status in
skaffold.deploy.Resource.Status
@codecov
Copy link

codecov bot commented Jun 24, 2020

Codecov Report

Merging #4390 into master will decrease coverage by 0.07%.
The diff coverage is 56.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4390      +/-   ##
==========================================
- Coverage   71.94%   71.87%   -0.08%     
==========================================
  Files         325      325              
  Lines       12708    12722      +14     
==========================================
+ Hits         9143     9144       +1     
- Misses       2989     3000      +11     
- Partials      576      578       +2     
Impacted Files Coverage Δ
pkg/diag/validator/pod.go 1.22% <0.00%> (-0.06%) ⬇️
pkg/diag/validator/resource.go 0.00% <0.00%> (ø)
pkg/skaffold/deploy/status_check.go 51.79% <55.55%> (-0.38%) ⬇️
pkg/skaffold/deploy/resource/status.go 85.71% <77.77%> (-4.29%) ⬇️
pkg/skaffold/deploy/resource/deployment.go 76.85% <79.48%> (+2.36%) ⬆️
pkg/skaffold/event/event.go 90.84% <100.00%> (-0.03%) ⬇️
...affold/kubernetes/portforward/kubectl_forwarder.go 60.97% <0.00%> (-2.44%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4459eea...270899b. Read the comment docs.

pkg/diag/validator/pod.go Outdated Show resolved Hide resolved
pkg/diag/validator/pod.go Outdated Show resolved Hide resolved
pkg/diag/validator/pod.go Show resolved Hide resolved
pkg/diag/validator/pod.go Outdated Show resolved Hide resolved
return r.ae
}
func (r Resource) StatusUpdated(another Resource) bool {
return r.ae.ErrCode != another.ae.ErrCode
Copy link
Member

Choose a reason for hiding this comment

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

Should this code check if r.ae != nil && another.ae != nil ?

pkg/skaffold/event/event.go Show resolved Hide resolved
pkg/skaffold/event/event.go Show resolved Hide resolved
@tejal29 tejal29 merged commit fc1b5da into GoogleContainerTools:master Jun 25, 2020
@tejal29 tejal29 deleted the add_infra_errors 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