Skip to content

Conversation

@LawnGnome
Copy link
Contributor

@LawnGnome LawnGnome commented Jan 20, 2021

This builds on #433 to handle the case where a user explicitly selects -workspace volume: we have to inspect the first container when creating the workspace when that happens, so we might as well do that anyway and keep the best workspace detector simple.

The major catch here is that workspace creators now have to have knowledge of the campaign spec steps, but I don't really see a clean way around that.

Implementation wise, this splits out our Docker image handling into a new internal/campaigns/docker package. This creates a fairly noisy looking diff, but it's not wildly different from what we had conceptually; it just unifies some of the details and simplifies the code in internal/campaigns, particularly in Service.

Fixes #432.

Copy link
Contributor

@mrnugget mrnugget left a comment

Choose a reason for hiding this comment

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

Approve because I don't see anything that's wrong with this approach.

I'm not too concerned about the []Step being passed to the workspace creators, since I'm pretty sure that more information would be required in the workspace creators as soon as we support the "run steps in subdirectory" functionality. We'll probably want to bundle repo and steps and zip and other stuff into workspaceParams or something.

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.

This seems good to me. Besides Thorstens comments, I don't have much to add. Seems to fix the issue and at some point we would have to do deeper inspection anyways I assume, so this coupling seems fair.

LawnGnome added a commit that referenced this pull request Jan 21, 2021
LawnGnome added a commit that referenced this pull request Jan 21, 2021
@LawnGnome LawnGnome force-pushed the aharvey/accio-workspace-owner branch from 9dc2bcf to de59a38 Compare January 21, 2021 03:49
@LawnGnome LawnGnome force-pushed the aharvey/accio-workspace-owner branch from f8ad0b5 to 9f8b853 Compare January 22, 2021 02:19
@LawnGnome LawnGnome marked this pull request as ready for review January 22, 2021 02:23
@LawnGnome
Copy link
Contributor Author

This is ready for review. Note that it currently includes #440; I intend to merge that first, then this. (Sorry for the review noise.)

@LawnGnome
Copy link
Contributor Author

I have merged #440, so disregard that. The thing that has changed since draft reviews is mostly adding testing, so that's where I'd appreciate some extra review work, in spite of the previous approvals. Thanks!

Copy link
Contributor

@mrnugget mrnugget left a comment

Choose a reason for hiding this comment

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

Cool! Tested it locally too, with the pin-docker-images and a hello-world campaign, with both bind and volume (using Docker for mac 3.0.1)

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.

Still LGTM :) Thanks for adding such a ton of tests!

@LawnGnome LawnGnome merged commit c8817bf into main Jan 22, 2021
@LawnGnome LawnGnome deleted the aharvey/accio-workspace-owner branch January 22, 2021 19:55
scjohns pushed a commit that referenced this pull request Apr 24, 2023
scjohns pushed a commit that referenced this pull request Apr 24, 2023
This reverts commit a35c2e7.

Co-authored-by: Thorsten Ball <mrnugget@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pin-docker-images2 failing with src version 3.23.3

4 participants