Skip to content
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

Build should take a context.Context #80

Closed
jonjohnsonjr opened this issue Sep 10, 2019 · 11 comments
Closed

Build should take a context.Context #80

jonjohnsonjr opened this issue Sep 10, 2019 · 11 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@jonjohnsonjr
Copy link
Collaborator

Builds can take a while, so it would be nice to be able to cancel them.

@jonjohnsonjr jonjohnsonjr added the good first issue Good for newcomers label Oct 22, 2019
@stanleynguyen
Copy link
Contributor

Hi @jonjohnsonjr Can I take this?

@jonjohnsonjr
Copy link
Collaborator Author

Sure!

@stanleynguyen
Copy link
Contributor

stanleynguyen commented Oct 30, 2019

@jonjohnsonjr I have a question. Does this refer to the private method build.build or the public one build.Build?

@jonjohnsonjr
Copy link
Collaborator Author

The public method, but I suspect we'll have to change both so that we can use https://godoc.org/os/exec#CommandContext

@stanleynguyen
Copy link
Contributor

Does that mean we will take another flag argument for the timeout duration? Because I was thinking something along the line of ctrl+C

@jonjohnsonjr
Copy link
Collaborator Author

Does that mean we will take another flag argument for the timeout duration?

I don't think we need a timeout. Truth be told, I don't use contexts for cancellation much, so I'm not sure what the idiomatic way to do this would be. My thinking was similar to yours (for ctrl-c), but I haven't done that before.

I stumbled across https://github.com/buildpack/pack/blob/9306cf3e129977b57d1ee8d57a1c1ac814a66898/commands/commands.go#L50-L61 when poking around in the buildpacks project, which seems similar to what we want, but again there might be a better way.

@stanleynguyen
Copy link
Contributor

stanleynguyen commented Nov 1, 2019

I was thinking of simply localizing it to the private build function, something like

signals := make(chan os.Signal)
signal.Notify(signals, syscall.SIGINT, syscall.SIGTERM)
ctx, cancel := context.WithCancel(context.Background())

go func() {
	<-signals
	cancel()
}()
cmd := exec.CommandContext(ctx, "go", args...)

Reason being that cmd package (nor any other external packages) doesn't need to know about the context and build doesn't need to get any info from the cmd package as well (unless we're to implement timeout)

@jonjohnsonjr
Copy link
Collaborator Author

jonjohnsonjr commented Nov 1, 2019

Reason being that cmd package (nor any other external packages) doesn't need to know about the context

My thinking here was specifically for external packages. Before cutting a 1.0 release, I thought we'd want to add a context to the API, since Build operations can take a long time and clients might want to cancel them... I guess this probably also applies to Publish.

I might be misunderstanding the purpose of context, though. I see that it's usually request oriented?

I was thinking of simply localizing it to the private build function, something like

I think that's fine for ko the binary, but it's a bit strange for ko the library. If you wanted to build something on top of this package, it's a bit weird for a library to register signal handlers. That's usually something I'd do in the main package and then propagate down through the library via the context.

I appreciate the back and forth here -- I'm not entirely sure what the best path forward here is, so any other ideas are helpful.

@stanleynguyen
Copy link
Contributor

@jonjohnsonjr Ohhh u're right about that. I didn't think of using ko as a library.
I will move forward with accepting Context as a param where it applies (not necessarily only Build function) and listen for the signal from the cmd. Does this sound good?

@jonjohnsonjr
Copy link
Collaborator Author

Does this sound good?

Yeah that sounds great! Hopefully it's not too much trouble :)

@jonjohnsonjr
Copy link
Collaborator Author

Fixed by #105

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants