[v3] Add validator in render v2. #5942
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5942 +/- ##
==========================================
+ Coverage 70.57% 70.59% +0.02%
==========================================
Files 457 458 +1
Lines 17531 17564 +33
==========================================
+ Hits 12372 12399 +27
- Misses 4244 4250 +6
Partials 915 915
Continue to review full report at Codecov.
|
yuwenma
force-pushed
the
render-4-validate
branch
from
4c48902 to
1347f8c
Compare
| cmd := exec.CommandContext(ctx, "kpt", "pkg", "init", outputPath) | ||
| kptFilePath := filepath.Join(r.hydrationDir, kptfile.KptFileName) | ||
| if _, err := os.Stat(kptFilePath); os.IsNotExist(err) { | ||
| cmd := exec.CommandContext(ctx, "kpt", "pkg", "init", r.hydrationDir) | ||
| if _, err := util.RunCmdOut(cmd); err != nil { | ||
| return err |
There was a problem hiding this comment.
Is this a kpt user error ? Can we define function and error codes like
and user kpt user error elsewhere.There was a problem hiding this comment.
can't guarantee it is a user error or not. I updated the err message with user actions.
There was a problem hiding this comment.
Is there a way to identify all possible if this was a kpt pkg init error or user config error?
Does kpt return unique exit codes based on different errors?
We are trying to attach an error code for all cases. You can also define KPT_PKG_INIT as an error code and return here.
There was a problem hiding this comment.
that's a good point. I'm not sure if kpt returns the different exit codes or not. We may need more user data to know how likely a user config error may happen in kpt pkg init or kpt fn
| defer file.Close() | ||
| kfConfig := &kptfile.KptFile{} | ||
| if err := yaml.NewDecoder(file).Decode(&kfConfig); err != nil { | ||
| return fmt.Errorf("unable to parse %v\n, please check if the kptfile is updated to new apiVersion > v1alpha2", |
There was a problem hiding this comment.
thanks for creating an actionable error. Can you use sErrors.New() and add error code as well as suggestion code to this error.
Docs here
https://github.com/GoogleContainerTools/skaffold/blob/master/DEVELOPMENT.md#adding-actionable-error-messages-to-code
The advantage of this this is, IDEs can consume the suggestion code and decide to show an actionable error message to the user as is or relevant one.
There was a problem hiding this comment.
oh, if the Decode fails, it basically means the yaml is invalid, it's not a skaffold error. the suggestion I added is just one possibility but not suggesting the error must be due to the API version difference. I'm afraid if using the suggestion code, it may mislead users. FYI, I attach the Decode error to the return value.
There was a problem hiding this comment.
we still need an error code with an actionable error message KPT_INVALID_YAML or KPT_USER_ERROR so we can track these in skaffold and get metrics on it.
Right, if a skaffold dev fails, the error code logged via metrics pipeline will be DEPLOY_UNKNOWN. We are trying to reduce these overall.
There was a problem hiding this comment.
I see. I added some TODO comments (here and other errors in ./render). Will add the Suggestion and error code in the following CL.
yuwenma
force-pushed
the
render-4-validate
branch
from
0835455 to
d602906
Compare
|
Updated. Please take another look @tejal29 |
There are two approaches to read Kptfile. One is via templating which we can add comments to help users understand the Kptfile. The other is via go struct which is more accurate and less error-prone. Considering the reliability of the Kpt deprecation policies (the api schema is not expected to change and if it is changed it will be backward compatible. If not backward compatible, the deprecation has a year-long period)
yuwenma
force-pushed
the
render-4-validate
branch
from
d602906 to
e6360ce
Compare
yuwenma
force-pushed
the
render-4-validate
branch
from
e6360ce to
e7bc1ee
Compare
yuwenma
force-pushed
the
render-4-validate
branch
from
e7bc1ee to
67fe089
Compare
Fixes: #nnn
Related: #5673
Merge after: #5940
Description
render.generateis not given in skaffold.yamlTo reviewers:
This PR is diff-based from 5940. Please only review the last commit