-
Notifications
You must be signed in to change notification settings - Fork 49
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
Go113errors #160
Go113errors #160
Conversation
Codecov Report
@@ Coverage Diff @@
## master #160 +/- ##
==========================================
+ Coverage 43.95% 44.35% +0.40%
==========================================
Files 56 61 +5
Lines 3133 3206 +73
==========================================
+ Hits 1377 1422 +45
- Misses 1381 1413 +32
+ Partials 375 371 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
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.
Thanks for doing all this!
LGTM pending addressing the nil-check issue for SemanticEqual.
There is one other nit about when to generate the list of known targets.
Lastly, you may want to change the state of this PR away from "draft" if it's ready for review.
147810a
to
efe1d10
Compare
constraint/pkg/client/errors.go
Outdated
|
||
// Errors is a list of error. | ||
// | ||
// Deprecated: Use a structured result type if it is important to disambiguate |
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.
Does this mean have individual custom types for each 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.
It's very situational. If we want to report individual error codes, then we should use a structured error type. If we need to programmatically work with information the error can communicate, then the error should have that structure. In general we want to treat errors as black-box as possible unless we have a functional reason to do otherwise.
Remove ErrorList as it complicates code and results in repetitive errors as implemented. Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Also use cmp.Diff instead of reflect.DeepEqual Signed-off-by: Will Beason <willbeason@google.com>
Make tests easily adaptable to multiple targets Signed-off-by: Will Beason <willbeason@google.com>
Cover paths for go113 errors to ensure correct errors are returned. Signed-off-by: Will Beason <willbeason@google.com>
It would return that Constraints were unequal if they both had no specs. This is not intended behavior. Also add unit tests. Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Use RLock for reading and Lock for writing. As-is could result in concurrent reads and writes. Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
54e8b74
to
a60513d
Compare
Move pkg/client to use go113 errors conventions.
In general, move to using constant base errors and wrap them with fmt.Errorf. Also make tests check that the expected error is returned.
In many cases I've extended tests to make them more flexible to accommodate this. I've made the tests more flexible, but haven't added examples of using that flexibility (for example, a client working with multiple targets). I intend to do so in a separate PR.
Fix a few concurrency bugs - the RW mutex wasn't properly guarding writes vs. reads for constraints. Also consider two Constraints with empty Specs to be "Semantically equal" as it is valid for Constraints to have empty specs.