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

add SandboxBuilder image override #50

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

devsnek
Copy link

@devsnek devsnek commented Nov 24, 2020

@devsnek
Copy link
Author

devsnek commented Nov 24, 2020

cc @pietroalbini

If this needs tests, where should they go?

@devsnek devsnek marked this pull request as ready for review November 24, 2020 20:18
Copy link
Member

@pietroalbini pietroalbini left a comment

Choose a reason for hiding this comment

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

Thanks for opening the PR! Left a few comments.

src/build.rs Outdated Show resolved Hide resolved
src/cmd/sandbox.rs Outdated Show resolved Hide resolved
@pietroalbini
Copy link
Member

Regarding tests, I'd add a tests/integration/sandbox_images.rs test that sets the workspace sandbox image to ubuntu:latest, tries to run cat /etc/os-release to ensure it's Ubuntu, then runs a command with debian:latest checking it's Debian instead.

@devsnek devsnek force-pushed the sandbox-image-override branch 2 times, most recently from 12c0f9e to b0eba00 Compare November 26, 2020 20:34
src/build.rs Outdated
@@ -171,6 +171,13 @@ impl BuildDirectory {
Ok(())
}

/// Get the path to the source code on the host machine (outside the sandbox).
pub fn get_source_dir(&self, krate: &Crate) -> Result<PathBuf, Error> {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I'd still just make the Crate::copy_source_to method public instead of an approach like th is.

Copy link
Author

@devsnek devsnek Nov 27, 2020

Choose a reason for hiding this comment

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

Where does one get the target dir from? Should I also make BuildDirectory::source_dir() public? krate.copy_source_to(&workspace, ???)

Copy link
Member

Choose a reason for hiding this comment

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

Docs.rs would have to create a temporary dir (for example with the tempfile crate) to extract the source code to.

src/cmd/sandbox.rs Outdated Show resolved Hide resolved
@devsnek devsnek force-pushed the sandbox-image-override branch 2 times, most recently from dde39c4 to e5081cb Compare November 27, 2020 19:06
if size > size_limit {
return Err(CommandError::SandboxImageTooLarge(size));
}
Some(m.config.digest)
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any config or digest field with docker manifest inspect. What version of the docker CLI were you using?

> docker manifest inspect ros:latest | jq .config
null
> docker manifest inspect ros:latest
{
   "schemaVersion": 2,
   "mediaType": "application/vnd.docker.distribution.manifest.list.v2+json",
   "manifests": [
      {
         "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
         "size": 3042,
         "digest": "sha256:ba0d9de623ad01a96d420140fed94cfb8de5e75be31cc33f0592d46e10dcccee",
         "platform": {
            "architecture": "amd64",
            "os": "linux"
         }
      },
      {
         "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
         "size": 3041,
         "digest": "sha256:42152a7c40961b39313cd0268a8d0aff483b56e817c784515a5b57cf2f2d1f28",
         "platform": {
            "architecture": "arm64",
            "os": "linux",
            "variant": "v8"
         }
      }
   ]
}

Copy link
Member

Choose a reason for hiding this comment

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

19.03.6

Copy link
Member

Choose a reason for hiding this comment

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

Wait, with that version I can't see the config part either.

Copy link
Member

Choose a reason for hiding this comment

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

> docker --version
Docker version 19.03.8, build afacb8b7f0

So I'm worried this doesn't actually work.

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.

3 participants