chore: safe type assersions #7770
Conversation
pkg/skaffold/parser/config.go
Outdated
| config, ok := cfg.(*latest.SkaffoldConfig) | ||
| if !ok { | ||
| log.Entry(context.TODO()).Debugf("failed interface conversion: %v is %T not *latest.SkaffoldConfig", cfg, cfg) | ||
| return nil, nil, fmt.Errorf("failed interface conversion: %v is %T not *latest.SkaffoldConfig", cfg, cfg) |
There was a problem hiding this comment.
This looks perfect. Can you instead of returning an error, can you return an sErrors.ActionableErr.
Please add a new error code for configuration parsing error here
skaffold/proto/enums/enums.proto
Line 505 in fb6b62e
CONFIG_UPGRADE_ERR?
Here is the documentation on how to add new error codes.
https://github.com/GoogleContainerTools/skaffold/blob/main/DEVELOPMENT.md#adding-actionable-error-messages-to-code
There was a problem hiding this comment.
I was thinking
CONFIG_FILE_PARSING_ERR
There was a problem hiding this comment.
@tejal29 I just added an actionable error. Let me know if this suffices or I still need to create the new actionable error?
There was a problem hiding this comment.
We want this error code to be unique to distinguish between
- when skaffold.yaml is correct
- when skaffold code failed to upgrade the config
Lets create a new one if its ok with you?
Codecov Report
@@ Coverage Diff @@
## main #7770 +/- ##
==========================================
- Coverage 70.48% 66.63% -3.86%
==========================================
Files 515 590 +75
Lines 23150 28504 +5354
==========================================
+ Hits 16317 18993 +2676
- Misses 5776 8120 +2344
- Partials 1057 1391 +334
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Hi @tejal29, I have added an actionable error. Thanks |
|
Thank you for your contribution!! Will merge if tests are green |
|
Please feel free to change this PR "Ready for Review" |
OladapoAjala
force-pushed
the
fix-panics-in-interface-conversions
branch
from
9f4121a to
ce3da76
Compare
Fixes: #7672
Related: Relevant tracking issues, for context
Merge before/after: Dependent or prerequisite PRs
Description