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

campaigns: only pull the Docker image in volume mode #477

Merged
merged 2 commits into from
Feb 22, 2021

Conversation

LawnGnome
Copy link
Contributor

This code went through a few iterations when we were developing it: originally, we only pulled the helper Docker image if volume mode was actually in use, but then we needed additional heuristics to determine when to enable volume mode based on the campaign's Docker images, which created a circular dependency between the Docker images and the workspace Docker image.

However, something changed right at the end of this that I hadn't fully thought through: originally, the workspace creator defines what Docker image(s) it required, but that was simplified into the service itself at some point. Additionally, pulling the helper image became a separate step in SetDockerImages, whereas before it was part of the same loop used for pulling campaign images.

The heuristic in bestWorkspaceCreator() requires that the Docker images be "set" within the campaign spec, but with those changes, we now actually have the right state in place within SetDockerImages to check which workspace creator will be used, so we've come full circle: we can prevent the helper image from being pulled if we know it won't be used.

Or, in effect: you can ensure that the only Docker images that will be pulled by src when executing a campaign are encoded in the campaign spec with this change if you use -workspace bind on your command line, accepting that this may be slower on macOS and Windows than the default.

Fixes https://github.com/sourcegraph/customer/issues/213.

@LawnGnome LawnGnome requested a review from a team February 22, 2021 23:24
@LawnGnome LawnGnome self-assigned this Feb 22, 2021
Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, easier than expected!

@LawnGnome LawnGnome merged commit 9464877 into main Feb 22, 2021
@LawnGnome LawnGnome deleted the aharvey/selectively-download-volume-workspace branch February 22, 2021 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants