Fix macOS panic by pulling images before determining workspace creator #522
Conversation
I introduced this when re-organizing the code in `batch_common.go`, but I didn't run into this because I've only developed on Linux since. On macOS, though, `src` would panic when the `DockerVolumeWorkspaceCreator` would check the `ImageUIDGUID` on each step, because `step.image` was nil. This fixes the issue by making sure that all images are pulled before inspecting them. As a requirement to the change it also pulls out the pulling of the workspace creator image, which I think is actually an improvement because it makes the code less decoupled. Guess I should switch between macOS and Linux more often.
| if workspaceCreator.Type() == workspace.CreatorTypeVolume { | ||
| _, err = svc.EnsureImage(ctx, workspace.DockerVolumeWorkspaceImage) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
This is an implementation detail I didn't really want to expose outside of the service and workspace code. (And went to a fair bit of trouble not to.)
If we need a separate workspace prep step, how do you feel about at least making it generic and letting Service figure out if it needs to do anything?
There was a problem hiding this comment.
This is an implementation detail I didn't really want to expose outside of the service and workspace code. (And went to a fair bit of trouble not to.)
My point for pulling this out is that it actually hides the "determine creator type" in the workspace package and gives us a single place in which the creator is setup and made ready. Previously Service did this in 3 steps: detected workspace type, downloaded workspace image with the other steps, the initialized the workspace. And it required the opts.flags.workspace to be set on Service.
Besides that I would argue that it wasn't an implementation detail, because the code had to be in a certain order for it to work and from the outside it was hard to see that the workspace creator wouldn't work if the images on the steps haven't been set. (And the progress bar for downloading the steps didn't include the workspace image, now the "determine workspace type" spinner runs for as long as the image is being pulled)
If we need a separate workspace prep step, how do you feel about at least making it generic and letting
Servicefigure out if it needs to do anything?
You mean like putting this into service and then do something like svc.BuildWorkspaceCreator? No strong opposition to that, although right now I'm leaning towards pulling things out of Service because the dependencies get really wishy-washy once a second layer is introduced. I also feel there's another layer/thing missing here: Service is a really good wrapper around Client, but I think there should be something like a coordinator or something that sits between the (terminal) UI and the service/executor/etc. and coordinates when things are called. Service, I think, started out as that thing, but with the introduction of the TUI we (very wisely) decided not to mix UI and Service and we ended up with Service being the things that coordinates other things but is also being coordinated by the TUI/user-flow.
Sorry for the raw thought dump here - I've just been knee deep in this and have been thinking about this a lot.
Short version: sure, not opposed, but I think there might be a better way to structure this code in general, but I haven't found/implemented it yet 😄
There was a problem hiding this comment.
Yeah, that's totally fair. OK, let's do it this way and see if we end up tripping on it when we do server side execution.
Thanks for giving this some thought!
#522) I introduced this when re-organizing the code in `batch_common.go`, but I didn't run into this because I've only developed on Linux since. On macOS, though, `src` would panic when the `DockerVolumeWorkspaceCreator` would check the `ImageUIDGUID` on each step, because `step.image` was nil. This fixes the issue by making sure that all images are pulled before inspecting them. As a requirement to the change it also pulls out the pulling of the workspace creator image, which I think is actually an improvement because it makes the code less decoupled. Guess I should switch between macOS and Linux more often.
I introduced this when re-organizing the code in
batch_common.go, butI didn't run into this because I've only developed on Linux since.
On macOS, though,
srcwould panic when theDockerVolumeWorkspaceCreatorwould check theImageUIDGUIDon eachstep, because
step.imagewas nil.This fixes the issue by making sure that all images are pulled before
inspecting them.
As a requirement to the change it also pulls out the pulling of the
workspace creator image, which I think is actually an improvement
because it makes the code less decoupled.
Guess I should switch between macOS and Linux more often.