Skip to content

Conversation

fanzhangio
Copy link

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 21, 2018
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 21, 2018
Copy link
Contributor

@Liujingfang1 Liujingfang1 left a comment

Choose a reason for hiding this comment

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

Good work. I'm expecting to see we can can use this error handler to cover all the places of parsing YAML file.

Path: filepath,
ErrorMsg: errorMsg,
}
if testErr != expectErr {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using expectErr as a YamlFormatError, can you use an expected error message?

expected := "some expected error message"
if testErr.Error() != expected {

Copy link
Author

Choose a reason for hiding this comment

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

Done

@@ -119,7 +120,7 @@ func NewResourceSliceFromPatches(

res, err := newResourceSliceFromBytes(content)
if err != nil {
return nil, err
return nil, internal.ErrorHandler(err, path)
Copy link
Contributor

Choose a reason for hiding this comment

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

There might be other places we parse YAML files and catch the error. Can you also apply this error handler for them?

Copy link
Author

Choose a reason for hiding this comment

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

Sure

@fanzhangio fanzhangio force-pushed the issue114 branch 2 times, most recently from f1a9bf4 to ef57f40 Compare June 22, 2018 18:04
`
)

//type foo struct
Copy link
Contributor

Choose a reason for hiding this comment

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

move this line to the bottom?

- add yaml format error handler
- silent usage when build command fails
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants