-
Notifications
You must be signed in to change notification settings - Fork 21
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
usability: Handle Exceptions for no resources #103
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #103 +/- ##
===========================================
- Coverage 67.66% 67.20% -0.46%
===========================================
Files 22 22
Lines 1633 1662 +29
===========================================
+ Hits 1105 1117 +12
- Misses 520 535 +15
- Partials 8 10 +2
Continue to review full report at Codecov.
|
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.
Awesome first unit test. 🎉 😺
This is a cool use of a function for writing out errors predictably. PTAL some minor suggestions for the UT.
pkg/blockdevice/blockdevice.go
Outdated
} else { | ||
// Show the output using cli-runtime | ||
util.TablePrinter(util.BDTreeListColumnDefinations, rows, printers.PrintOptions{Wide: true}) | ||
} |
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.
Dang, I'm positive golangCI-lint isn't running all the tools now, I'll will create a good-first-issue
for the same.
What should have happened if CI was working optimally ?
The build should have ideally failed because we have added GolangCI-lint to our CI, I cannot recall the exact warning text but the functionality won't change even if the else bracket is removed, the tool which does it likely gosimple.
func awesome() {
....
if something {
return nil, err
} else {
return value, nil
}
} // end of function
can get simplified to
func awesome() {
....
if something {
return nil, err
}
return value
} // end of function
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.
Corrected.
pkg/util/error.go
Outdated
@@ -40,3 +40,16 @@ func CheckErr(err error, handleErr func(string)) { | |||
} | |||
handleErr(err.Error()) | |||
} | |||
|
|||
// Handle Empty Table 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.
// Handle Empty Table Error | |
// HandleEmptyTableError handles errors when a resource or set of resources aren't found |
This is a minor linting issue in golang, exported functions must be properly documented as such.
Ref: https://go.dev/blog/godoc
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.
Sounds great, Done
pkg/util/error.go
Outdated
return fmt.Errorf("no %s found in your cluster", resource) | ||
} else if ns != "" && casType != "" { | ||
return fmt.Errorf("no %s %s found in %s namespace", casType, resource, ns) | ||
} else if casType != "" { |
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.
Abhinandan has recently written a nice utility function which returns true for valid casTypes. Try to rebase your code against the current origin/develop
and you'd likely see that in this package. Use that one for this condition check.
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.
Done
Signed-off-by: Abhishek-kumar09 <abhimait1909@gmail.com>
Signed-off-by: Abhishek-kumar09 <abhimait1909@gmail.com>
Signed-off-by: Abhishek-kumar09 <abhimait1909@gmail.com>
ae51c8b
to
acf45c5
Compare
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.
lgtm
Signed-off-by: Abhishek-kumar09 abhimait1909@gmail.com
Fixes #56
Now empty resource lists are handled by the openebsctl for storage, volumes and bd. They are handled separately on the basis of resourceType, casType and Namespaces.