bake: do not fail printing definition if instance unavailable #1345
Conversation
|
@tonistiigi With this repro #1344 (comment) I found out that completing the trace span for the bake command on return timed out (10s). Looks to be this defer: Lines 35 to 37 in 105c214 Line 26 in 105c214 |
That's a broader subject but yeah we need integration tests for the cli and mock some components like the docker/cli. |
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
| // instance only needed for reading remote bake files or building | ||
| if url != "" || !in.printOnly { |
There was a problem hiding this comment.
I wonder if a better way to write this would be to fetch the instance "lazily"? So that we only call getInstanceOnDefault to get dis at the point of use?
There was a problem hiding this comment.
Doing these things lazily definitely seems nice. Not sure how much would be involved in doing that (and if the current patch fixes the issue, perhaps we should consider the "easy" fix at least as a temporary solution)
There was a problem hiding this comment.
Yeah sounds good 🎉
The logic around drivers keeps getting hairier and hairier, we should definitely look into some cleanup soon 👀
There was a problem hiding this comment.
I have another branch to mutualize the builder/driver logic so we could lazily load an instance. Not yet ready but you can take a look: master...crazy-max:buildx:builder-pkg
fixes #1344
Since 0.9 we are checking the context builder endpoint to make sure it's reachable before returning the instance: #1129 (comment). By doing so we are not soft-failing as before when loading
driversForNodeGroupand thereforebake --printnow fails if no instance is available.This PR will not return an error if we are printing local bake files. An instance is still required when building or loading remote bake files.
Signed-off-by: CrazyMax crazy-max@users.noreply.github.com