-
Notifications
You must be signed in to change notification settings - Fork 547
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
fix panic when os.Stat returns an error besides ErrNotExists #2162
Conversation
Signed-off-by: Samsondeen Dare <samsondeen.dare@hashicorp.com>
Codecov Report
@@ Coverage Diff @@
## main #2162 +/- ##
==========================================
+ Coverage 26.23% 26.70% +0.47%
==========================================
Files 130 131 +1
Lines 7617 7688 +71
==========================================
+ Hits 1998 2053 +55
- Misses 5362 5374 +12
- Partials 257 261 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Looks like this needs the boilerplate fix! |
Signed-off-by: Samsondeen Dare <samsondeen.dare@hashicorp.com>
e2e0c47
to
5d2b5d3
Compare
pkg/cosign/common.go
Outdated
@@ -27,12 +27,15 @@ import ( | |||
) | |||
|
|||
// TODO(jason): Move this to an internal package. | |||
func FileExists(filename string) bool { | |||
func FileExists(filename string) (bool, 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.
What do other semver folks think about this change? It's technically breaking, but I can't find any usage of it on GitHub.
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.
Good catch. Also, to Jason's (Todo) point, I think this should be in an internal package, as this behaviour doesn't need to be public to users. It's trivial to implement if they need to. That would prevent us from running into issues with breaking semver.
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.
Yeah, if we're going to change the interface we might as well move it internal at the same time to prevent future issues. WDYT about making the 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.
Sure. I can make a commit to that effect. Thanks
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.
+1
4e3efd0
to
9d30c9e
Compare
9d30c9e
to
83d2391
Compare
Signed-off-by: Samsondeen Dare <samsondeen.dare@hashicorp.com>
83d2391
to
bf79f0d
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.
small request
Signed-off-by: Samsondeen Dare <samsondeen.dare@hashicorp.com>
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
thanks so much
will defer to @dlorenc for final approval
…e#2162) * fix panic when os.Stat returns an error besides ErrNotExists Signed-off-by: Samsondeen Dare <samsondeen.dare@hashicorp.com> * boilerplate fix Signed-off-by: Samsondeen Dare <samsondeen.dare@hashicorp.com> * move FileExists to internal package Signed-off-by: Samsondeen Dare <samsondeen.dare@hashicorp.com> * remove archived pkg/errors Signed-off-by: Samsondeen Dare <samsondeen.dare@hashicorp.com> * comments Signed-off-by: Samsondeen Dare <samsondeen.dare@hashicorp.com> * comments Signed-off-by: Samsondeen Dare <samsondeen.dare@hashicorp.com> Signed-off-by: Samsondeen Dare <samsondeen.dare@hashicorp.com>
Signed-off-by: Samsondeen Dare samsondeen.dare@hashicorp.com
Summary
This PR fixes a nil pointer error that occurs when
os.Stat
returns an error besidesErrNotExist
. The issue was also reported in #2161Release Note
bug fix: fix panic when os.Stat returns an error besides ErrNotExists
Documentation