-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat: add 'skaffold inspect jobManifestPath' and 'skaffold transform-schema jobManifestPath' commands #8575
feat: add 'skaffold inspect jobManifestPath' and 'skaffold transform-schema jobManifestPath' commands #8575
Conversation
9f89eb5
to
3b16ae3
Compare
5e7f659
to
032eab9
Compare
032eab9
to
179842a
Compare
Codecov Report
@@ Coverage Diff @@
## main #8575 +/- ##
==========================================
- Coverage 70.48% 64.21% -6.27%
==========================================
Files 515 613 +98
Lines 23150 30689 +7539
==========================================
+ Hits 16317 19707 +3390
- Misses 5776 9505 +3729
- Partials 1057 1477 +420
... and 391 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
13078b2
to
218dcf3
Compare
// Open our jsonFile | ||
jsonFile, err := os.Open(inputFile) | ||
if err != nil { | ||
fmt.Println(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we return the err here? Or log it with log.Entry(context.TODO()).Error
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, good catch. This now properly handles errors and has a test case related to this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also logs the error as well to stdout for users to know what happened
byteValue, _ := ioutil.ReadAll(jsonFile) | ||
|
||
var result jobManifestPathList | ||
json.Unmarshal(byteValue, &result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This operation can return an error, if the inputFile
is not a json file. I noticed that we fail silently, and the generated output is the same skaffold.yaml without the modifications. Should we also log the error from the unmarshal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, good catch. This now properly handles errors and has a test case related to this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also logs the error as well to stdout for users to know what happened
218dcf3
to
4995397
Compare
…schema jobManifestPath' commands
4995397
to
16b84ff
Compare
fixes #8573