-
Notifications
You must be signed in to change notification settings - Fork 47
fix: state cache falls through to slow path on error #885
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #885 +/- ##
======================================
Coverage 31.2% 31.2%
======================================
Files 39 39
Lines 3865 3865
======================================
Hits 1209 1209
Misses 2510 2510
Partials 146 146 |
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.
Nice! This is ready to merge once comments are addressed.
@@ -1,54 +0,0 @@ | |||
# This file was generated with `make docker-files` and should not |
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.
You're probably aware - undo this change.
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.
Hmm. make clean deletes this file so what is the expected pattern for building? Do I need to regerenate the dockerfiles and recommit every time?
@@ -1,42 +0,0 @@ | |||
# This file was generated with `make docker-files` and should not |
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.
ditto
lens/util/store.go
Outdated
} | ||
|
||
if !value.(reflect.Value).Type().AssignableTo(o.Type()) { | ||
return xerrors.Errorf("out parameter cannot be assigned cached value") |
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.
include the types in the error message
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
lens/util/store.go
Outdated
func (cas *CachingStateStore) tryAssign(value interface{}, out interface{}) error { | ||
o := reflect.ValueOf(out).Elem() | ||
if !o.CanSet() { | ||
return xerrors.Errorf("out parameter cannot be set") |
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.
include type in error message
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
@frrist restored dockerfiles |
Looks good, my approval is sticky. |
Prevents cache failures causing extraction to fail