-
Notifications
You must be signed in to change notification settings - Fork 527
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
Plumb context through crane and gcrane #995
Conversation
Codecov Report
@@ Coverage Diff @@
## main #995 +/- ##
==========================================
- Coverage 75.24% 75.15% -0.09%
==========================================
Files 107 107
Lines 5058 5064 +6
==========================================
Hits 3806 3806
- Misses 706 712 +6
Partials 546 546
Continue to review full report at Codecov.
|
@@ -29,5 +29,5 @@ func Catalog(src string, opt ...Option) (res []string, err error) { | |||
return nil, err | |||
} | |||
|
|||
return remote.Catalog(context.TODO(), reg, o.remote...) | |||
return remote.Catalog(context.Background(), reg, o.remote...) |
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.
Not plumbing context through here?
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.
The remote.WithContext
in o.remote...
will override it.
@@ -61,8 +62,9 @@ func main() { | |||
root.AddCommand(cmd) | |||
} | |||
|
|||
if err := root.Execute(); err != nil { | |||
fmt.Println(err) | |||
ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt) |
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.
defer cancel () instead?
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.
Doesn't os.Exit(1)
pre-empt it?
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.
ended up doing both instead
053a029
to
3750ef0
Compare
Also, use signal.NotifyContext for cancelling the ctx.
Fixes #912
Also, use signal.NotifyContext for cancelling the ctx.
Also, don't log errors twice now that we're using RunE.